summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMiklos Szeredi <mszeredi@redhat.com>2020-12-14 17:26:14 +0300
committerMiklos Szeredi <mszeredi@redhat.com>2020-12-14 17:26:14 +0300
commit89bdfaf93d9157499c3a0d61f489df66f2dead7f (patch)
tree3917eaff11b85be312054dfaf44b8a6a60e6b966
parentc846af050f944d584f28bc0de310383003c8096d (diff)
downloadlinux-89bdfaf93d9157499c3a0d61f489df66f2dead7f.tar.xz
ovl: make ioctl() safe
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
-rw-r--r--fs/overlayfs/file.c87
1 files changed, 16 insertions, 71 deletions
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index efccb7c1f9bc..a1f72ac053e5 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -541,46 +541,31 @@ static long ovl_real_ioctl(struct file *file, unsigned int cmd,
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 file *file, unsigned int cmd,
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, unsigned int cmd, unsigned long arg)
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: