On Wed, Jul 10, 2024 at 02:23:23PM +0200, Mickaël Salaün 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.

Looking at existing LSM implementations, even if some helpers (e.g.
selinux_inode) return NULL if inode->i_security is NULL, this may not be
handled by the callers.  For instance, SELinux always dereferences the
blob pointer in the security_inode_permission() hook.  EVM seems to be
the only one properly handling this case.

Shouldn't we remove all inode->i_security checks and assume it is always
set?  This is currently the case anyway, but it would be clearer this
way and avoid false sense of security (with useless checks).

A patch was sent to do this kind of check:
https://lore.kernel.org/r/20140109101932.0508dec7@gandalf.local.home (local)
but the applied commit 3dc91d4338d6 ("SELinux: Fix possible NULL pointer
dereference in selinux_inode_permission()") didn't include the
i_security check.

Since this commit, the security_inode_free()'s header comment is no
longer correct because i_security is no longer set to NULL.