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:
> > 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.
>
> Yeah, I don't either...
>
> > > */
> > > 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...
>
> >
> > > 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@xxxxxxxxxxxxxxxxxx/
> (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...
>
> >
> > > */
> > > 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.
>
>
> diff --git a/security/security.c b/security/security.c
> index 9c3fb2f60e2a..a8658ebcaf0c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1613,7 +1613,8 @@ static void inode_free_by_rcu(struct rcu_head *head)
> */
> void security_inode_free(struct inode *inode)
> {
> - call_void_hook(inode_free_security, inode);
> + struct rcu_head *inode_blob = inode->i_security;
> +
> /*
> * The inode may still be referenced in a path walk and
> * a call to security_inode_permission() can be made
> @@ -1623,9 +1624,11 @@ void security_inode_free(struct inode *inode)
> * leave the current inode->i_security pointer intact.
> * The inode will be freed after the RCU grace period too.
> */
> - if (inode->i_security)
> - call_rcu((struct rcu_head *)inode->i_security,
> - inode_free_by_rcu);
> + if (inode_blob) {
> + call_void_hook(inode_free_security, inode);
> + inode->i_security = NULL;
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);
> + }
> }
