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.
... 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.
> > */
> > 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.
> 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.
--
paul-moore.com
