Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security

["Mickaël Salaün" <mic@digikod.net>]

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@gandalf.local.home/
> (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);
> +	}
>  }