Re: [PATCH v5 0/6] evm: Prepare for moving to the LSM infrastructure

[Mimi Zohar]

On Wed, 2022-11-23 at 13:44 +0100, Roberto Sassu wrote:
> On Wed, 2022-11-23 at 07:28 -0500, Mimi Zohar wrote:
> > Hi Roberto,
> > 
> > On Wed, 2022-11-23 at 10:51 +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > 
> > > One of the challenges that must be tackled to move IMA and EVM to the LSM
> > > infrastructure is to ensure that EVM is capable to correctly handle
> > > multiple stacked LSMs providing an xattr at file creation. At the moment,
> > > there are few issues that would prevent a correct integration. This patch
> > > set aims at solving them.
> > 
> > Let's take a step back and understand the purpose of this patch set. 
> > Regardless of whether IMA and EVM are moved to the "LSM
> > infrastructure", EVM needs to support per LSM xattrs.  A side affect is
> > the removal of the security_old_inode_init_security hook.  This patch
> > set cover letter and patch descriptions should be limited to EVM
> > support for per LSM (multiple) xattrs.  The motivation, concerns, and
> > problems of making IMA and EVM LSMs will be documented in the patch set
> > that actual makes them LSMs.  Please remove all references to "move IMA
> > and EVM to the LSM infrastructure".
> 
> Hi Mimi
> 
> ok, will do.
> 
> > When EVM was upstreamed, there were filesystem limitations on the
> > number and size of the extended attributes.  In addition there were
> > performance concerns, which resulted in staging the LSM, IMA and EVM
> > xattrs, before calling initxattrs to write them at the same time.  With
> > this patch set, not only are per LSM xattrs supported, but multiple per
> > LSM xattrs are supported as well.  Have the size limitation concerns
> > been addressed by the different filesystems?   If not, then at minimum
> > this patch set needs to at least mention it and the possible
> > ramifications.
> 
> With your patch, 9d8f13ba3f483 ("security: new
> security_inode_init_security API adds function callback") you made it
> possible to set multiple xattrs at inode creation time.

True, and even then there were concerns.

> This patch set pushes further to the limits, as there could be more
> xattrs to be added to the inode. I will mention that.

Thanks

> If there are too many xattrs, I guess the only solution would be to use
> less LSMs, or a different filesystem. The per filesystem limit could be
> increased separately case by case.

Agreed, but unless it is documented somewhere, nobody but us will know
there is a potential problem.  At least document it here in the cover
letter, which we'll include in the merge message.

FYI, the xattr.7 man page contains a section "Filesystem differences".

-- 
thanks,

Mimi