[PATCH v5 23/23] integrity: Switch from rbtree to LSM-managed blob for integrity_iint_cache

[Roberto Sassu]

On Wed, 2023-11-29 at 13:27 +0100, Roberto Sassu wrote:
> On Mon, 2023-11-20 at 16:06 -0500, Paul Moore wrote:
> > On Mon, Nov 20, 2023 at 3:16 AM Roberto Sassu
> > <roberto.sassu at huaweicloud.com> wrote:
> > > On Fri, 2023-11-17 at 15:57 -0500, Paul Moore wrote:
> > > > On Nov  7, 2023 Roberto Sassu <roberto.sassu at huaweicloud.com> wrote:
> > > > > 
> > > > > Before the security field of kernel objects could be shared among LSMs with
> > > > > the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> > > > > of inode metadata. The association between inode metadata and inode is
> > > > > maintained through an rbtree.
> > > > > 
> > > > > Because of this alternative storage mechanism, there was no need to use
> > > > > disjoint inode metadata, so IMA and EVM today still share them.
> > > > > 
> > > > > With the reservation mechanism offered by the LSM infrastructure, the
> > > > > rbtree is no longer necessary, as each LSM could reserve a space in the
> > > > > security blob for each inode. However, since IMA and EVM share the
> > > > > inode metadata, they cannot directly reserve the space for them.
> > > > > 
> > > > > Instead, request from the 'integrity' LSM a space in the security blob for
> > > > > the pointer of inode metadata (integrity_iint_cache structure). The other
> > > > > reason for keeping the 'integrity' LSM is to preserve the original ordering
> > > > > of IMA and EVM functions as when they were hardcoded.
> > > > > 
> > > > > Prefer reserving space for a pointer to allocating the integrity_iint_cache
> > > > > structure directly, as IMA would require it only for a subset of inodes.
> > > > > Always allocating it would cause a waste of memory.
> > > > > 
> > > > > Introduce two primitives for getting and setting the pointer of
> > > > > integrity_iint_cache in the security blob, respectively
> > > > > integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> > > > > the code more understandable, as they directly replace rbtree operations.
> > > > > 
> > > > > Locking is not needed, as access to inode metadata is not shared, it is per
> > > > > inode.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu at huawei.com>
> > > > > Reviewed-by: Casey Schaufler <casey at schaufler-ca.com>
> > > > > Reviewed-by: Mimi Zohar <zohar at linux.ibm.com>
> > > > > ---
> > > > >  security/integrity/iint.c      | 71 +++++-----------------------------
> > > > >  security/integrity/integrity.h | 20 +++++++++-
> > > > >  2 files changed, 29 insertions(+), 62 deletions(-)
> > > > > 
> > > > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > > > > index 882fde2a2607..a5edd3c70784 100644
> > > > > --- a/security/integrity/iint.c
> > > > > +++ b/security/integrity/iint.c
> > > > > @@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
> > > > >     return 0;
> > > > >  }
> > > > > 
> > > > > +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> > > > > +   .lbs_inode = sizeof(struct integrity_iint_cache *),
> > > > > +};
> > > > 
> > > > I'll admit that I'm likely missing an important detail, but is there
> > > > a reason why you couldn't stash the integrity_iint_cache struct
> > > > directly in the inode's security blob instead of the pointer?  For
> > > > example:
> > > > 
> > > >   struct lsm_blob_sizes ... = {
> > > >     .lbs_inode = sizeof(struct integrity_iint_cache),
> > > >   };
> > > > 
> > > >   struct integrity_iint_cache *integrity_inode_get(inode)
> > > >   {
> > > >     if (unlikely(!inode->isecurity))
> > > >       return NULL;
> > > >     return inode->i_security + integrity_blob_sizes.lbs_inode;
> > > >   }
> > > 
> > > It would increase memory occupation. Sometimes the IMA policy
> > > encompasses a small subset of the inodes. Allocating the full
> > > integrity_iint_cache would be a waste of memory, I guess?
> > 
> > Perhaps, but if it allows us to remove another layer of dynamic memory
> > I would argue that it may be worth the cost.  It's also worth
> > considering the size of integrity_iint_cache, while it isn't small, it
> > isn't exactly huge either.
> > 
> > > On the other hand... (did not think fully about that) if we embed the
> > > full structure in the security blob, we already have a mutex available
> > > to use, and we don't need to take the inode lock (?).
> > 
> > That would be excellent, getting rid of a layer of locking would be significant.
> > 
> > > I'm fully convinced that we can improve the implementation
> > > significantly. I just was really hoping to go step by step and not
> > > accumulating improvements as dependency for moving IMA and EVM to the
> > > LSM infrastructure.
> > 
> > I understand, and I agree that an iterative approach is a good idea, I
> > just want to make sure we keep things tidy from a user perspective,
> > i.e. not exposing the "integrity" LSM when it isn't required.
> 
> Ok, I went back to it again.
> 
> I think trying to separate integrity metadata is premature now, too
> many things at the same time.
> 
> I started to think, does EVM really need integrity metadata or it can
> work without?
> 
> The fact is that CONFIG_IMA=n and CONFIG_EVM=y is allowed, so we have
> the same problem now. What if we make IMA the one that manages
> integrity metadata, so that we can remove the 'integrity' LSM?
> 
> So, no embedding the full structure in the security blob now, move
> integrity_inode_free() and integrity_kernel_module_request() to IMA,
> call integrity_iintcache_init() from IMA.
> 
> EVM verification of new files would fail without IMA, but it would be
> the same now.
> 
> Also, evm_verifyxattr() would only work with IMA, as it assumes that
> the latter creates integrity metadata and passes them as argument.
> 
> Regarding the LSM order, I would take Casey's suggestion of introducing
> LSM_ORDER_REALLY_LAST, for EVM.

I attach the diff v5..v7.

Tests passes with both IMA and EVM enabled. I did minor tweaks to the
tests to take into account the possibility that IMA is disabled, and
tests pass also in this case.

Roberto