On Wed, Jul 10, 2024 at 8:23 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.

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.

Since SELinux requires space in inode->i_security there should never
be a case where SELinux is enabled and we don't allocate a blob for
inode->i_security.

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

It would be interesting to draft a patch to do that and make sure
everything still works; it *should*, but I'd want to see that change
get some good testing :)

Keep in mind, this still doesn't mean that an LSM is required to use
any space in inode->i_security if it wants to use the inode hooks.

-- 
paul-moore.com