[RFC PATCH] lsm: add the inode_free_security_rcu() LSM implementation hook

[Casey Schaufler]

On 7/10/2024 9:20 AM, Paul Moore wrote:
> On Wed, Jul 10, 2024 at 6:40 AM Mickaël Salaün <mic at digikod.net> wrote:
>> On Tue, Jul 09, 2024 at 10:40:30PM -0400, Paul Moore wrote:
>>> The LSM framework has an existing inode_free_security() hook which
>>> is used by LSMs that manage state associated with an inode, but
>>> due to the use of RCU to protect the inode, special care must be
>>> taken to ensure that the LSMs do not fully release the inode state
>>> until it is safe from a RCU perspective.
>>>
>>> This patch implements a new inode_free_security_rcu() implementation
>>> hook which is called when it is safe to free the LSM's internal inode
>>> state.  Unfortunately, this new hook does not have access to the inode
>>> itself as it may already be released, so the existing
>>> inode_free_security() hook is retained for those LSMs which require
>>> access to the inode.
>>>
>>> Signed-off-by: Paul Moore <paul at paul-moore.com>
>> I like this new hook.  It is definitely safer than the current approach.
> It would be nicer to simply have a single inode free hook, but it
> doesn't look like that is a possibility due to the inode
> synchronization methods and differing lifetimes of inodes and their
> associated LSM state.  At least the workaround isn't too ugly :)
>
>> To make it more consistent, I think we should also rename
>> security_inode_free() to security_inode_put() to highlight the fact that
>> LSM implementations should not free potential pointers in this blob
>> because they could still be dereferenced in a path walk.
> I'd prefer to keep the naming as-is in this patch since
> security_inode_free() does trigger the free'ing/release of the LSM
> state associated with the inode.
>
>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>> index 22d8b7c28074..f583f8cec345 100644
>>> --- a/security/landlock/fs.c
>>> +++ b/security/landlock/fs.c
>>> @@ -1198,13 +1198,16 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>>>
>>>  /* Inode hooks */
>>>
>>> -static void hook_inode_free_security(struct inode *const inode)
>>> +static void hook_inode_free_security_rcu(void *inode_sec)
>>>  {
>>> +     struct landlock_inode_security *lisec;
>> Please rename "lisec" to "inode_sec" for consistency with
>> get_inode_object()'s variables.
> Done.  That did conflict with the parameter name so I renamed the
> parameter everywhere to "inode_security".
>
>>>       /*
>>>        * All inodes must already have been untied from their object by
>>>        * release_inode() or hook_sb_delete().
>>>        */
>>> -     WARN_ON_ONCE(landlock_inode(inode)->object);
>>> +     lisec = inode_sec + landlock_blob_sizes.lbs_inode;
>>> +     WARN_ON_ONCE(lisec->object);
>>>  }
>> This looks good to me.
>>
>> We can add these footers:
>> Reported-by: syzbot+5446fbf332b0602ede0b at syzkaller.appspotmail.com
>> Closes: https://lore.kernel.org/r/00000000000076ba3b0617f65cc8@google.com
> Thanks, metadata added.  This was a quick RFC patch so I didn't want
> to spend a lot of time chasing down metadata refs until I knew there
> was some basic support for this approach.  I still want to make sure
> it is okay with the IMA folks.
>
>> However, I'm wondering if we could backport this patch down to v5.15 .
>> I guess not, so I'll need to remove this hook implementation for
>> Landlock, backport it to v5.15, and then you'll need to re-add this
>> check with this patch.  At least it has been useful to spot this inode
>> issue, but it could still be useful to spot potential memory leaks with
>> a negligible performance impact.
> Yes, it's a bit complicated with the IMA/EVM promotion happening
> fairly recently.  I'm marking the patch with a stable tag, but
> considering we're at -rc7 and this needs at least one more respin,
> testing, ACKs, etc. it might not land in Linus' tree until sometime
> post v6.11-rc1.  Considering that, I might suggest dropping the
> Landlock hook in -stable and re-adding it to Linus' tree once this fix
> lands, but that decision is up to you.
>
>>> diff --git a/security/security.c b/security/security.c
>>> index b52e81ac5526..bc6805f7332e 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -1596,9 +1596,8 @@ int security_inode_alloc(struct inode *inode)
>>>
>>>  static void inode_free_by_rcu(struct rcu_head *head)
>>>  {
>>> -     /*
>>> -      * The rcu head is at the start of the inode blob
>>> -      */
>>> +     /* The rcu head is at the start of the inode blob */
>>> +     call_void_hook(inode_free_security_rcu, head);
>> For this to work, we need to extend the inode blob size (lbs_inode) with
>> sizeof(struct rcu_head).  The current implementation override the
>> content of the blob with a new rcu_head.
> Take a look at lsm_set_blob_sizes() and you'll see that the rcu_head
> struct is already accounted for in the inode->i_security blob.
>
>>> @@ -1606,20 +1605,21 @@ static void inode_free_by_rcu(struct rcu_head *head)
>>>   * security_inode_free() - Free an inode's LSM blob
>>>   * @inode: the inode
>>>   *
>>> - * Deallocate the inode security structure and set @inode->i_security to NULL.
>>> + * Release any LSM resources associated with @inode, although due to the
>>> + * inode's RCU protections it is possible that the resources will not be
>>> + * fully released until after the current RCU grace period has elapsed.
>>> + *
>>> + * It is important for LSMs to note that despite being present in a call to
>>> + * security_inode_free(), @inode may still be referenced in a VFS path walk
>>> + * and calls to security_inode_permission() may be made during, or after,
>>> + * a call to security_inode_free().  For this reason the inode->i_security
>>> + * field is released via a call_rcu() callback and any LSMs which need to
>>> + * retain inode state for use in security_inode_permission() should only
>>> + * release that state in the inode_free_security_rcu() LSM hook callback.
>>>   */
>>>  void security_inode_free(struct inode *inode)
>>>  {
>>>       call_void_hook(inode_free_security, inode);
>>> -     /*
>>> -      * The inode may still be referenced in a path walk and
>>> -      * a call to security_inode_permission() can be made
>>> -      * after inode_free_security() is called. Ideally, the VFS
>>> -      * wouldn't do this, but fixing that is a much harder
>>> -      * job. For now, simply free the i_security via RCU, and
>>> -      * leave the current inode->i_security pointer intact.
>>> -      * The inode will be freed after the RCU grace period too.
>>> -      */
>>>       if (inode->i_security)
>>>               call_rcu((struct rcu_head *)inode->i_security,
>>>                        inode_free_by_rcu);
>> We should have something like:
>> call_rcu(inode->i_security.rcu, inode_free_by_rcu);
> The unfortunate side effect of that is that the patch grows
> significantly as everyone that touches inode->i_security would need to
> be updated to use inode->i_security.data or something similar.  For a
> patch that we likely want to see backported to -stable that could make
> things more difficult than needed.
>
> However, I have always thought we should add some better
> structure/typing to these opaque LSM blobs both to get away from the
> raw pointer math and add a marginal layer of safety.  I've envisioned
> doing something like this:
>
>   struct lsm_blob_inode {
>     struct selinux_blob_inode selinux;
>     struct smack_blob_inode smack;
>     struct aa_blob_inode apparmor;
>     ...
>     struct rcu_head rcu;
>   }

I have considered doing this as part of the stacking effort for quite
some time. I've already done it for the lsmblob structure that will replace
most uses of the u32 secid in the LSM APIs. I am concerned that there would
be considerable gnashing of teeth over the potential increase in the blob
sizes for kernels compiled with LSMs that aren't active. I have been frantically
avoiding anything that might slow the stacking effort further. If this would
help moving this along I'll include it in v40.

>
> .. with lsm_blob_inode allocated and assigned to inode->i_security.
> Those LSMs that are enabled and require blob space would define their
> struct with the necessary fields, those that were disabled or did not
> require space would define an empty struct; some minor pre-processor
> checking and header file work would be needed, but it shouldn't be too
> bad.  Ideally, we would use the same approach for all of the LSM
> security blobs, only with different LSM supplied structs.  Perhaps
> once we land Casey's latest patchset I'll see about doing something
> like that, but right now we've got bigger problems to address.
>
> I'll hold off on posting a proper patch for this RFC until I hear back
> from Mini or Roberto on the IMA changes.
>