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

[Casey Schaufler]

On 12/13/2023 2:45 AM, Roberto Sassu wrote:
> On 17.11.23 21:57, Paul Moore wrote:
>> On Nov  7, 2023 Roberto Sassu <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
>>>
>>> ...
>>>
>>> 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?

Unless there's a case where inodes are created without calling
security_inode_alloc() there should never be an inode without a
security blob by the time you get to the Smack hook. That said,
people seem inclined to take all sorts of shortcuts and create
various "inodes" that aren't really inodes. I also see that SELinux
doesn't check the blob for cred or file structures. And that I
wrote the code in both cases.

Based on lack of bug reports for Smack on inodes and SELinux on
creds or files, It appears that the check is unnecessary. On the
other hand, it sure looks like good error detection hygiene. I
would be inclined to include the check in new code, but not get
in a panic about existing code.

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