Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security

[Christian Brauner <brauner@kernel.org>]

On Mon, Jul 08, 2024 at 04:11:41PM GMT, Mickaël Salaün 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@digikod.net> 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()?
> I replied here instead:
> https://lore.kernel.org/r/20240708.hohNgieja0av@digikod.net
> 
> > 
> > ... 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?
> 
> > 
> > > >        */
> > > >       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.
> 
> > 
> > > 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").

Path walking can happen in RCU- and ref-mode. We'll try RCU first and
fallback to ref mode if it fails with ECHILD. In RCU mode it's possible
to call inode_permission() on nd->inode (e.g., in may_lookup()) while
dentry->d_inode has changed (rename or similar)). This would be detected
later when we validate that the parent child relationship hasn't changed
while we were under RCU and would force a drop out of RCU into ref
walking mode.