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>
>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/r/00000000000076ba3b0617f65cc8@xxxxxxxxxx
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;
}
... 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.
--
paul-moore.com
