On Wed, Jul 10, 2024 at 12:20:18PM -0400, Paul Moore wrote:
> On Wed, Jul 10, 2024 at 6:40 AM Mickaël Salaün <mic@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > 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".
Oh, I didn't see this conflict. Using inode_security as function
argument looks good.
> > 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.
I would prefer to backport the new hook. I can help with that. In
fact, I tried to backport the removal of the hook for Landlock, and it
requires the same amount of work, so it would be better to work
together. That should also ease future backports impacting the same
part of the code.
BTW, while trying to backport that to linux-5.15.y I noticed that a fix
is missing in this branch: the default return value for the
inode_init_security hook, see commit 6bcdfd2cac55 ("security: Allow all
LSMs to provide xattrs for inode_init_security hook").
>
> > > 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.
Indeed, I was confused with the lsm_set_blob_size() name which *adds* a
size.
>
> > > @@ -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.
Sure, this was related to my confusion with the rcu_head size addition.
