Re: [PATCH v2 04/10] ovl: make ioctl() safe
From: Amir Goldstein <amir73il@gmail.com>
Date: 2020-12-08 11:12:48
Also in:
linux-fsdevel, linux-unionfs, lkml
On Mon, Dec 7, 2020 at 6:36 PM Miklos Szeredi [off-list ref] wrote:
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 <redacted>
Looks reasonable Reviewed-by: Amir Goldstein <amir73il@gmail.com>
quoted hunk ↗ jump to hunk
--- fs/overlayfs/file.c | 75 ++------------------------------------------- 1 file changed, 3 insertions(+), 72 deletions(-)diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index efccb7c1f9bc..3cd1590f2030 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c@@ -541,46 +541,26 @@ 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) ret = vfs_ioctl(real.file, cmd, arg); - revert_creds(old_cred); fdput(real); return ret; }
I wonder if we shouldn't leave a comment behind to explain that no override is intentional. I also wonder if "Permission model" sections shouldn't be saying something about ioctl() (current task checks only)? or we just treat this is a breakage of the permission model that needs to be fixed? Thanks, Amir.