linux-kernel - Re: [PATCH v3 25/25] 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: <d0afbd03940e45219852787a1001d8debe48bf09.camel@huaweicloud.com>
Date:   Fri, 13 Oct 2023 13:31:55 +0200
From:   Roberto Sassu <roberto.sassu@...weicloud.com>
To:     Stefan Berger <stefanb@...ux.ibm.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, zohar@...ux.ibm.com, dmitry.kasatkin@...il.com,
        paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
        dhowells@...hat.com, jarkko@...nel.org,
        stephen.smalley.work@...il.com, eparis@...isplace.org,
        casey@...aufler-ca.com
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-nfs@...r.kernel.org, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, keyrings@...r.kernel.org,
        selinux@...r.kernel.org, Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [PATCH v3 25/25] integrity: Switch from rbtree to LSM-managed
 blob for integrity_iint_cache

On Fri, 2023-09-15 at 11:39 +0200, Roberto Sassu wrote:
> On Tue, 2023-09-12 at 12:19 -0400, Stefan Berger wrote:
> > On 9/4/23 09:40, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@...wei.com>
> > > 
> > > 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.
> > > 
> > > 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. Thus, request from the 'integrity' LSM a
> > > space in the security blob for the pointer of inode metadata
> > > (integrity_iint_cache structure).
> > > 
> > > Prefer this 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>
> > > ---
> > > 
> > > @@ -145,10 +91,8 @@ static void integrity_inode_free(struct inode *inode)
> > >   	if (!IS_IMA(inode))
> > >   		return;
> > 
> > I think you can remove this check !IS_IMA()  as well since the next 
> > function called here integrity_iint_find() already has this check:
> > 
> > struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
> > {
> >          if (!IS_IMA(inode))
> >                  return NULL;
> > 
> >          return integrity_inode_get_iint(inode);
> > }
> 
> I agree, thanks!

Actually, I had to keep it otherwise, without a check on iint,
iint_free() can get NULL as argument.

Roberto

> Roberto
> 
> > >   
> > > -	write_lock(&integrity_iint_lock);
> > > -	iint = __integrity_iint_find(inode);
> > > -	rb_erase(&iint->rb_node, &integrity_iint_tree);
> > > -	write_unlock(&integrity_iint_lock);
> > > +	iint = integrity_iint_find(inode);         <--------------
> > > +	integrity_inode_set_iint(inode, NULL);
> > >   
> > >   	iint_free(iint);
> > >   }
>