linux-ext4 - Re: [PATCH v2] vfs: only allow SETFLAGS to set DAX flag on files and dirs

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: <20210721232818.GB8639@magnolia>
Date:   Wed, 21 Jul 2021 16:28:18 -0700
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     JeffleXu <jefflexu@...ux.alibaba.com>
Cc:     viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org
Subject: Re: [PATCH v2] vfs: only allow SETFLAGS to set DAX flag on files and
 dirs

On Tue, Jul 20, 2021 at 03:33:20PM +0800, JeffleXu wrote:
> 
> 
> On 7/20/21 1:43 AM, Darrick J. Wong wrote:
> > On Mon, Jul 19, 2021 at 10:38:34AM +0800, Jeffle Xu wrote:
> >> This is similar to commit dbc77f31e58b ("vfs: only allow FSSETXATTR to
> >> set DAX flag on files and dirs").
> >>
> >> Though the underlying filesystems may have filtered invalid flags, e.g.,
> >> ext4_mask_flags() called in ext4_fileattr_set(), also check it in VFS
> >> layer.
> >>
> >> Signed-off-by: Jeffle Xu <jefflexu@...ux.alibaba.com>
> >> ---
> >> changes since v1:
> >> - add separate parentheses surrounding flag tests
> >> ---
> >>  fs/ioctl.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/ioctl.c b/fs/ioctl.c
> >> index 1e2204fa9963..90cfaa4db03a 100644
> >> --- a/fs/ioctl.c
> >> +++ b/fs/ioctl.c
> >> @@ -835,7 +835,7 @@ static int fileattr_set_prepare(struct inode *inode,
> >>  	 * It is only valid to set the DAX flag on regular files and
> >>  	 * directories on filesystems.
> >>  	 */
> >> -	if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
> >> +	if (((fa->fsx_xflags & FS_XFLAG_DAX) || (fa->flags & FS_DAX_FL)) &&
> > 
> > Isn't fileattr_fill_flags supposed to fill out fa->fsx_xflags from
> > fa->flags for a SETFLAGS call?
> 
> Yes, but fa->fsx_xflags inherited from fa->flags (at least in ext4 it
> is) is the original flags/xflags of the file before SETFLAG/FSSETXATTR.

How?  old_ma is the original flags/xflags of the file.  fa reflects what
we copied in from userspace.  We use old_ma to set flags in fa that
couldn't possibly have been set by userspace, but neither DAX flag is in
that set.

Ugh, this is so much bookkeeping code to read it makes my head hurt.  Do
you have a reproducer?  I can't figure out how to trip this problem.

> Here we want to check *new* flags/xflags.

AFAICT, SETFLAGS will call ioctl_setflags, which will...
...read flags from userspace
...fill out struct fileattr via fileattr_fill_flags, which will set
   fa.fsx_flags from fa.flags, so the state of both fields' DAX flags
   will be whatever userspace gave us
...call vfs_fileattr_set, which will...
...call vfs_fileattr_get to fill out out_ma
...update the rest of xflags with the xflags from out_ma that weren't
   already set
...call fileattr_set_prepare, where it shouldn't matter if it checks
   (fa->xflags & FS_XFLAG_DAX) or (fa->flags & FS_DAX_FL), since they
   have the same value

FSSETXATTR will call ioctl_fssetxattr, which will...
...call copy_fsxattr_from_user to read fsxattr from userspace
...call fileattr_fill_xflags to set fa->flags from fa->xflags, so the
   state of both fields' DAX flags will be whatever userspace gave us
...call vfs_fileattr_set, which will...
...call vfs_fileattr_get to fill out out_ma
...update the rest of flags with the flags from out_ma that weren't
   already set
...call fileattr_set_prepare, where it shouldn't matter if it checks
   (fa->xflags & FS_XFLAG_DAX) or (fa->flags & FS_DAX_FL), since they
   have the same value

So where did I go wrong?

--D

> 
> > 
> >>  	    !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
> >>  		return -EINVAL;
> >>  
> >> -- 
> >> 2.27.0
> >>
> 
> -- 
> Thanks,
> Jeffle