linux-kernel - Re: [syzbot] [lsm?] general protection fault in hook_inode_free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240710.Hai0Uj3Phaij@digikod.net>
Date: Wed, 10 Jul 2024 14:23:21 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Paul Moore <paul@...l-moore.com>
Cc: Jann Horn <jannh@...gle.com>, Christian Brauner <brauner@...nel.org>, 
	"Paul E. McKenney" <paulmck@...nel.org>, Casey Schaufler <casey@...aufler-ca.com>, 
	Kees Cook <keescook@...omium.org>, syzbot <syzbot+5446fbf332b0602ede0b@...kaller.appspotmail.com>, 
	jmorris@...ei.org, linux-kernel@...r.kernel.org, 
	linux-security-module@...r.kernel.org, serge@...lyn.com, syzkaller-bugs@...glegroups.com, 
	linux-fsdevel@...r.kernel.org, Mimi Zohar <zohar@...ux.ibm.com>, 
	Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [syzbot] [lsm?] general protection fault in
 hook_inode_free_security

On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote:
> On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün <mic@...ikod.net> 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.
> 
> 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! :)
> 
> > >  */
> > > 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).