Re: XFS: Assertion failed

[Yumei Huang <yuhuang@redhat.com>]

----- Original Message -----
> From: "Dave Chinner" <david@fromorbit.com>
> To: "Brian Foster" <bfoster@redhat.com>
> Cc: "Yumei Huang" <yuhuang@redhat.com>, linux-xfs@vger.kernel.org
> Sent: Friday, January 15, 2021 5:21:02 AM
> Subject: Re: XFS: Assertion failed
> 
> On Thu, Jan 14, 2021 at 12:29:28PM -0500, Brian Foster wrote:
> > On Thu, Jan 14, 2021 at 05:20:29AM -0500, Yumei Huang wrote:
> > > Hit the issue when doing syzkaller test with kernel 5.11.0-rc3(65f0d241).
> > > The C reproducer is attached.
> > > 
> > > Steps to Reproduce:
> > > 1. # gcc -pthread -o reproducer reproducer.c
> > > 2. # ./reproducer
> > > 
> > > 
> > > Test results:
> > > [  131.726790] XFS: Assertion failed: (iattr->ia_valid &
> > > (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> > > ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0, file:
> > > fs/xfs/xfs_iops.c, line: 849
> > > [  131.743687] ------------[ cut here ]------------
> > 
> > Some quick initial analysis from a run of the reproducer... It looks
> > like it calls into xfs_setattr_size() with ATTR_KILL_PRIV set in
> > ->ia_valid. This appears to originate in the VFS via handle_truncate()
> > -> do_truncate() -> dentry_needs_remove_privs().
> > 
> > An strace of the reproducer shows the following calls:
> > 
> > ...
> > [pid  1524] creat("./file0", 010)       = 3
> > ...
> > [pid  1524] fsetxattr(3, "security.capability",
> > "\0\0\0\3b\27\0\0\10\0\0\0\2\0\0\0\377\377\377\377\0\356\0", 24, 0
> > <unfinished ...>
> > ...
> > [pid  1524] creat("./file0", 010 <unfinished ...>
> > ...
> > 
> > So I'm guessing there's an attempt to open this file with O_TRUNC with
> > this particular xattr set (unexpectedly?). Indeed, after the reproducer
> > leaves file01 around with the xattr, a subsequent xfs_io -c "open -t
> > ..." attempt triggers the assert again, and then the xattr disappears.
> > I'd have to dig more into the associated vfs code to grok the expected
> > behavior and whether there's a problem here..
> 
> Changing the size of the inode is is all that xfs_setattr_size()
> should be doing. Stripping capability attributes should have been
> already been done by the generic setattr code before we get to
> xfs_setattr_size(), so ATTR_KILL_PRIV should not be set at that
> point.
> 
> notify_change() used to always strip ATTR_KILL_PRIV from ia_valid
> when it sets up the flags necessary to strip privileges in the
> ->setattr callout. But it doesn't appear to do so always anymore:
> 
>         if (ia_valid & ATTR_KILL_PRIV) {
>                 error = security_inode_need_killpriv(dentry);
>                 if (error < 0)
>                         return error;
>                 if (error == 0)
>                         ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV;
>         }
> 
> If ATTR_KILL_PRIV is still set, this implies
> security_inode_need_killpriv() returned > 0 for some reason. I'm
> assuming that this code ran:
> 
> security_inode_need_killpriv()
>   call_int_hook(inode_need_killpriv, 0, dentry);
> 
> And the only implemented hook is this:
> 
> LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
> 
> /**
>  * cap_inode_need_killpriv - Determine if inode change affects privileges
>  * @dentry: The inode/dentry in being changed with change marked
>  ATTR_KILL_PRIV
>  *
>  * Determine if an inode having a change applied that's marked ATTR_KILL_PRIV
>  * affects the security markings on that inode, and if it is, should
>  * inode_killpriv() be invoked or the change rejected.
>  *
>  * Returns 1 if security.capability has a value, meaning inode_killpriv()
>  * is required, 0 otherwise, meaning inode_killpriv() is not required.
>  */
> int cap_inode_need_killpriv(struct dentry *dentry)
> {
>         struct inode *inode = d_backing_inode(dentry);
>         int error;
>                                                                                  
>         error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0);
>         return error > 0;
> }
> 
> And that's the xattr that the reproducer is setting. So, smoking
> gun.
> 
> we've done a lookup on the security.capability xattr which it
> found so notify_change() does not clear ATTR_KILL_PRIV. The xattr
> gets killed in setattr_prepare() but it does not clear
> ATTR_KILL_PRIV, and hence we hit the assert faili when we get into
> xfs_setattr_size.
> 
> Looks like a regression introduced in 2016 by:
> 
> commit 030b533c4fd4d2ec3402363323de4bb2983c9cee
> Author: Jan Kara <jack@suse.cz>
> Date:   Thu May 26 17:21:32 2016 +0200
> 
>     fs: Avoid premature clearing of capabilities
>     
>     Currently, notify_change() clears capabilities or IMA attributes by
>     calling security_inode_killpriv() before calling into ->setattr. Thus it
>     happens before any other permission checks in inode_change_ok() and user
>     is thus allowed to trigger clearing of capabilities or IMA attributes
>     for any file he can look up e.g. by calling chown for that file. This is
>     unexpected and can lead to user DoSing a system.
>     
>     Fix the problem by calling security_inode_killpriv() at the end of
>     inode_change_ok() instead of from notify_change(). At that moment we are
>     sure user has permissions to do the requested change.
>     
>     References: CVE-2015-1350
>     Reviewed-by: Christoph Hellwig <hch@lst.de>
>     Signed-off-by: Jan Kara <jack@suse.cz>
> 
> The bug is harmless, it will only trigger the assert on debug XFS
> kernels, but otherwise ATTR_KILL_PRIV is not checked/used by
> xfs_setattr_size.
> 
> Removing ATTR_KILL_PRIV from the assert is probably all that is
> needed. Can you write a patch for that, Yumei?

Sure, I will send the patch soon.


Best Regards,

Yumei Huang

> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> 
>