On Thu, Jun 27, 2024 at 11:12:43AM -0700, Kees Cook wrote:

On Thu, Jun 27, 2024 at 03:34:41PM +0200, Mickaël Salaün 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.

Yeah, I don't either...

quoted

quoted

 */
void security_inode_free(struct inode *inode)
{

Shouldn't we add this check here?
if (!inode->i_security)
	return;

Probably, yes. The LSMs that check for NULL i_security in the free hook
all do so right at the beginning...

quoted

quoted

	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.

Yeah, it's not clear to me what the expected lifetime is here. How is
inode_permission() being called if all inode reference counts are 0? It
does seem intentional, though.

The RCU logic was introduced in commit 3dc91d4338d6 ("SELinux: Fix possible
NULL pointer dereference in selinux_inode_permission()"), with much
discussion:
https://lore.kernel.org/lkml/20140109101932.0508dec7@gandalf.local.home/ (local)
(This commit seems to remove setting "i_security = NULL", though, which
the comment implies is intended, but then it also seems to depend on
finding a NULL?)

LSMs using i_security are:

security/bpf/hooks.c:   .lbs_inode = sizeof(struct bpf_storage_blob),
security/integrity/evm/evm_main.c:      .lbs_inode = sizeof(struct evm_iint_cache),
security/integrity/ima/ima_main.c:      .lbs_inode = sizeof(struct ima_iint_cache *),
security/landlock/setup.c:      .lbs_inode = sizeof(struct landlock_inode_security),
security/selinux/hooks.c:       .lbs_inode = sizeof(struct inode_security_struct),
security/smack/smack_lsm.c:     .lbs_inode = sizeof(struct inode_smack),

SELinux is still checking for NULL. See selinux_inode() and
selinux_inode_free_security(), as do bpf_inode() and
bpf_inode_storage_free(). evm and ima also check for NULL.

landlock_inode() does not, though.

Smack doesn't hook the free, but it should still check for NULL, and it's not.

So I think this needs fixing in Landlock and Smack.

I kind of think that the LSM infrastructure needs to provide a common
helper for the "access the blob" action, as we've got it repeated in
each LSM, and we have 2 implementations that are missing NULL checks...

quoted

quoted

	 */
	if (inode->i_security)
		call_rcu((struct rcu_head *)inode->i_security,
			 inode_free_by_rcu);

And then:
inode->i_security = NULL;

But why call_rcu()?  i_security is not protected by RCU barriers.

I assume it's because security_inode_free() via __destroy_inode() via
destroy_inode() via evict() via iput_final() via iput() may be running
in interrupt context?

But I still don't see where i_security gets set to NULL. This won't fix
the permissions hook races for Landlock and Smack, but should make
lifetime a bit more clear?

It should not change anything.  I don't see how inode->i_security can be
NULL and when such an inode can be passed to an LSM hook.

If a path walk is ongoing, couldn't this lead to an LSM's security check
bypass?  Shouldn't we call all the inode_free_security() hooks in
inode_free_by_rcu()?  That would mean to reserve an rcu_head and then
probably use inode->i_rcu instead.

I think your patch is correct though.  Could you please send a full
patch?

+		call_rcu(inode_blob, inode_free_by_rcu);
+	}
 }