On Mon, Jul 8, 2024 at 10:11 AM Mickaël Salaün [off-list ref] wrote:

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

quoted

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()?

My two concerns would be 1) an LSM needed to mark the inode
immediately (to be fair, I'm not sure how plausible that is, and it
would likely require LSM specific locking/sync) and 2) if an LSM
needed more context than would be valid in the RCU callback case
(although that is questionable, I don't believe any of the current
users need anything like that, e.g. task_struct).  I'm not opposed to
just moving everything to a single hook done when the inode is
actually dropped from memory as long as the current LSMs which make
use of it are okay with that (Landlock, IMA, SELinux).

quoted

... 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?

I poked around a bit more and it looks like bcachefs has some error
handling code that frees the inodes synchronously instead of using the
i_callback/inode::free_inode approach; xfs has its own thing too.
Although if it really is just some bcachefs/xfs error handlers maybe
inserting a LSM hook in the proper
the-memory-is-actually-being-dropped code path  (with adjustments for
bcachefs/xfs) isn't too bad.  Once again, we would need to hear from
the VFS folks regarding their thoughts on this, it wouldn't surprise
me if there is some detail missing in the above, or if they are
opposed to this for some other reason not mentioned yet in this
thread.

-- 
paul-moore.com