On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote:

On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün [off-list ref] wrote:

quoted

I didn't find specific issues with Landlock's code except the extra
check in hook_inode_free_security().  It looks like inode->i_security is
a dangling pointer, leading to UAF.

Reading security_inode_free() comments, two things looks weird to me:

quoted

/**
 * security_inode_free() - Free an inode's LSM blob
 * @inode: the inode
 *
 * Deallocate the inode security structure and set @inode->i_security to NULL.

I don't see where i_security is set to NULL.

The function header comments are known to be a bit suspect, a side
effect of being detached from the functions for many years, this may
be one of those cases.  I tried to fix up the really awful ones when I
moved the comments back, back I didn't have time to go through each
one in detail.  Patches to correct the function header comments are
welcome and encouraged! :)

quoted

quoted

 */
void security_inode_free(struct inode *inode)
{

Shouldn't we add this check here?
if (!inode->i_security)
        return;

Unless I'm remembering something wrong, I believe we *should* always
have a valid i_security pointer each time we are called, if not
something has gone wrong, e.g. the security_inode_free() hook is no
longer being called from the right place.  If we add a NULL check, we
should probably have a WARN_ON(), pr_err(), or something similar to
put some spew on the console/logs.

All that said, it would be good to hear some confirmation from the VFS
folks that the security_inode_free() hook is located in a spot such
that once it exits it's current RCU critical section it is safe to
release the associated LSM state.

It's also worth mentioning that while we always allocate i_security in
security_inode_alloc() right now, I can see a world where we allocate
the i_security field based on need using the lsm_blob_size info (maybe
that works today?  not sure how kmem_cache handled 0 length blobs?).
The result is that there might be a legitimate case where i_security
is NULL, yet we still want to call into the LSM using the
inode_free_security() implementation hook.

quoted

quoted

      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.

It's not clear to me why this should be safe if an LSM try to use the
partially-freed blob after the hook calls and before the actual blob
free.

I had the same thought while looking at this just now.  At least in
the SELinux case I think this "works" simply because SELinux doesn't
do much here, it just drops the inode from a SELinux internal list
(long story) and doesn't actually release any memory or reset the
inode's SELinux state (there really isn't anything to "free" in the
SELinux case).  I haven't checked the other LSMs, but they may behave
similarly.

We may want (need?) to consider two LSM implementation hooks called
from within security_inode_free(): the first where the existing
inode_free_security() implementation hook is called, the second inside
the inode_free_by_rcu() callback immediately before the i_security
data is free'd.

Couldn't we call everything in inode_free_by_rcu()?
I replied here instead:
https://lore.kernel.org/r/20240708.hohNgieja0av@digikod.net (local)

... or we find a better placement in the VFS for
security_inode_free(), is that is possible.  It may not be, our VFS
friends should be able to help here.

Christian? Al?

quoted

quoted

       */
      if (inode->i_security)
              call_rcu((struct rcu_head *)inode->i_security,
                       inode_free_by_rcu);

And then:
inode->i_security = NULL;

According to the comment we may still need i_security for permission
checks.  See my comment about decomposing the LSM implementation into
two hooks to better handle this for LSMs.

That was my though too, but maybe not if the path walk just ends early.

quoted

But why call_rcu()?  i_security is not protected by RCU barriers.

I believe the issue is that the inode is protected by RCU and that
affects the lifetime of the i_security blob.

It seems to be related to commit fa0d7e3de6d6 ("fs: icache RCU free
inodes").