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] [day] [month] [year] [list]

Date: Thu, 27 Jun 2024 15:29:48 -0400
From: Paul Moore <paul@...l-moore.com>
To: Kees Cook <kees@...nel.org>
Cc: Mickaël Salaün <mic@...ikod.net>, 
	Christian Brauner <brauner@...nel.org>, Steven Rostedt <rostedt@...dmis.org>, Jann Horn <jannh@...gle.com>, 
	"Paul E. McKenney" <paulmck@...nel.org>, Casey Schaufler <casey@...aufler-ca.com>, 
	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
Subject: Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security

On Thu, Jun 27, 2024 at 2:12 PM Kees Cook <kees@...nel.org> wrote:
> 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;
> +               call_rcu(inode_blob, inode_free_by_rcu);

See my response to Mickaël, unless we can get some guidance from the
VFS folks on the possibility of calling security_inode_free() once
when the inode has already been marked for death and is no longer in
use, I'd rather see us split the LSM implementation into two hooks,
something like this pseudo code (very hand-wavy around the rcu_head
inode container bits):

void inode_free_rcu(rhead)
{
  inode = multiple_container_of_magic(rhead);
  /* LSMs can finally free any internal state */
  call_void_hook(inode_free_rcu, inode)
  inode->i_security = NULL;
}

void security_inode_free(inode)
{
  /* LSMs can mark their state as "dead", but perm checks may still happen */
  call_void_hook(inode_free, inode);
  if (inode->i_security)
    call_rcu(inode, inode_free_rcu);
}

> +       }
>  }

--
paul-moore.com