Re: [PATCH v2 04/10] ovl: make ioctl() safe

[Amir Goldstein <amir73il@gmail.com>]

On Mon, Dec 7, 2020 at 6:36 PM Miklos Szeredi <mszeredi@redhat.com> 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 <mszeredi@redhat.com>

Looks reasonable

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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.