From: Miklos Szeredi mszeredi@redhat.com
commit 89bdfaf93d9157499c3a0d61f489df66f2dead7f upstream.
ovl_ioctl_set_flags() does a capability check using flags, but then the real ioctl double-fetches flags and uses potentially different value.
The "Check the capability before cred override" comment misleading: user can skip this check by presenting benign flags first and then overwriting them to non-benign flags.
Just remove the cred override for now, hoping this doesn't cause a regression.
The proper solution is to create a new setxflags i_op (patches are in the works).
Xfstests don't show a regression.
Reported-by: Dmitry Vyukov dvyukov@google.com Signed-off-by: Miklos Szeredi mszeredi@redhat.com Reviewed-by: Amir Goldstein amir73il@gmail.com Fixes: dab5ca8fd9dd ("ovl: add lsattr/chattr support") Cc: stable@vger.kernel.org # v4.19 Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
--- fs/overlayfs/file.c | 87 +++++++++------------------------------------------- 1 file changed, 16 insertions(+), 71 deletions(-)
--- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -541,46 +541,31 @@ static long ovl_real_ioctl(struct file * unsigned long arg) { struct fd real; - const struct cred *old_cred; long ret;
ret = ovl_real_fdget(file, &real); if (ret) return ret;
- old_cred = ovl_override_creds(file_inode(file)->i_sb); ret = security_file_ioctl(real.file, cmd, arg); - if (!ret) + if (!ret) { + /* + * Don't override creds, since we currently can't safely check + * permissions before doing so. + */ ret = vfs_ioctl(real.file, cmd, arg); - revert_creds(old_cred); + }
fdput(real);
return ret; }
-static unsigned int ovl_iflags_to_fsflags(unsigned int iflags) -{ - unsigned int flags = 0; - - if (iflags & S_SYNC) - flags |= FS_SYNC_FL; - if (iflags & S_APPEND) - flags |= FS_APPEND_FL; - if (iflags & S_IMMUTABLE) - flags |= FS_IMMUTABLE_FL; - if (iflags & S_NOATIME) - flags |= FS_NOATIME_FL; - - return flags; -} - static long ovl_ioctl_set_flags(struct file *file, unsigned int cmd, - unsigned long arg, unsigned int flags) + unsigned long arg) { long ret; struct inode *inode = file_inode(file); - unsigned int oldflags;
if (!inode_owner_or_capable(inode)) return -EACCES; @@ -591,10 +576,13 @@ static long ovl_ioctl_set_flags(struct f
inode_lock(inode);
- /* Check the capability before cred override */ - oldflags = ovl_iflags_to_fsflags(READ_ONCE(inode->i_flags)); - ret = vfs_ioc_setflags_prepare(inode, oldflags, flags); - if (ret) + /* + * Prevent copy up if immutable and has no CAP_LINUX_IMMUTABLE + * capability. + */ + ret = -EPERM; + if (!ovl_has_upperdata(inode) && IS_IMMUTABLE(inode) && + !capable(CAP_LINUX_IMMUTABLE)) goto unlock;
ret = ovl_maybe_copy_up(file_dentry(file), O_WRONLY); @@ -613,46 +601,6 @@ unlock:
}
-static long ovl_ioctl_set_fsflags(struct file *file, unsigned int cmd, - unsigned long arg) -{ - unsigned int flags; - - if (get_user(flags, (int __user *) arg)) - return -EFAULT; - - return ovl_ioctl_set_flags(file, cmd, arg, flags); -} - -static unsigned int ovl_fsxflags_to_fsflags(unsigned int xflags) -{ - unsigned int flags = 0; - - if (xflags & FS_XFLAG_SYNC) - flags |= FS_SYNC_FL; - if (xflags & FS_XFLAG_APPEND) - flags |= FS_APPEND_FL; - if (xflags & FS_XFLAG_IMMUTABLE) - flags |= FS_IMMUTABLE_FL; - if (xflags & FS_XFLAG_NOATIME) - flags |= FS_NOATIME_FL; - - return flags; -} - -static long ovl_ioctl_set_fsxflags(struct file *file, unsigned int cmd, - unsigned long arg) -{ - struct fsxattr fa; - - memset(&fa, 0, sizeof(fa)); - if (copy_from_user(&fa, (void __user *) arg, sizeof(fa))) - return -EFAULT; - - return ovl_ioctl_set_flags(file, cmd, arg, - ovl_fsxflags_to_fsflags(fa.fsx_xflags)); -} - long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { long ret; @@ -663,12 +611,9 @@ long ovl_ioctl(struct file *file, unsign ret = ovl_real_ioctl(file, cmd, arg); break;
- case FS_IOC_SETFLAGS: - ret = ovl_ioctl_set_fsflags(file, cmd, arg); - break; - case FS_IOC_FSSETXATTR: - ret = ovl_ioctl_set_fsxflags(file, cmd, arg); + case FS_IOC_SETFLAGS: + ret = ovl_ioctl_set_flags(file, cmd, arg); break;
default: