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

[Roberto Sassu <roberto.sassu@huaweicloud.com>]

On 17.11.23 21:57, Paul Moore wrote:
> On Nov  7, 2023 Roberto Sassu <roberto.sassu@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@huawei.com>
>> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
>> Reviewed-by: Mimi Zohar <zohar@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;

Ok, this caught my attention...

I see that selinux_inode() has it, but smack_inode() doesn't.

Some Smack code assumes that the inode security blob is always non-NULL:

static void init_inode_smack(struct inode *inode, struct smack_known *skp)
{
	struct inode_smack *isp = smack_inode(inode);

	isp->smk_inode = skp;
	isp->smk_flags = 0;
}


Is that intended? Should I add the check?

Thanks

Roberto

>      return inode->i_security + integrity_blob_sizes.lbs_inode;
>    }
> 
> --
> paul-moore.com