Commit 89bdfaf9 authored by Miklos Szeredi's avatar Miklos Szeredi
Browse files

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: default avatarDmitry Vyukov <dvyukov@google.com>
Signed-off-by: default avatarMiklos Szeredi <mszeredi@redhat.com>
Reviewed-by: default avatarAmir Goldstein <amir73il@gmail.com>
Fixes: dab5ca8f ("ovl: add lsattr/chattr support")
Cc: <stable@vger.kernel.org> # v4.19
parent c846af05

fs/overlayfs/file.c

+16 −71
Original line number Diff line number Diff line
@@ -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:

CRA Git | Maintained and supported by SUSTech CRA and CCSE