On Mon, Jul 8, 2024 at 10:11 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> 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 <mic@xxxxxxxxxxx> wrote:
> > >
> > > 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:
> > >
> > > > /**
> > > > * 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! :)
> >
> > > > */
> > > > 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.
> >
> > > > 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).
> > ... 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
