On Mon, Dec 7, 2020 at 6:36 PM Miklos Szeredi <mszeredi@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
Looks reasonable
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> 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.
