linux-kernel - Re: [PATCH v5 23/23] integrity: Switch from rbtree to LSM-managed blob for integrity_iint

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2084adba3c27a606cbc5ed7b3214f61427a829dd.camel@huaweicloud.com>
Date:   Mon, 20 Nov 2023 09:16:09 +0100
From:   Roberto Sassu <roberto.sassu@...weicloud.com>
To:     Paul Moore <paul@...l-moore.com>, viro@...iv.linux.org.uk,
        brauner@...nel.org, chuck.lever@...cle.com, jlayton@...nel.org,
        neilb@...e.de, kolga@...app.com, Dai.Ngo@...cle.com,
        tom@...pey.com, jmorris@...ei.org, serge@...lyn.com,
        zohar@...ux.ibm.com, dmitry.kasatkin@...il.com,
        dhowells@...hat.com, jarkko@...nel.org,
        stephen.smalley.work@...il.com, eparis@...isplace.org,
        casey@...aufler-ca.com, mic@...ikod.net
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-nfs@...r.kernel.org, linux-security-module@...r.kernel.org,
        linux-integrity@...r.kernel.org, keyrings@...r.kernel.org,
        selinux@...r.kernel.org, Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [PATCH v5 23/23] integrity: Switch from rbtree to LSM-managed
 blob  for integrity_iint_cache

On Fri, 2023-11-17 at 15:57 -0500, Paul Moore wrote:
> On Nov  7, 2023 Roberto Sassu <roberto.sassu@...weicloud.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@...wei.com>
> > Reviewed-by: Casey Schaufler <casey@...aufler-ca.com>
> > Reviewed-by: Mimi Zohar <zohar@...ux.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?

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 (?).

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.

Thanks

Roberto