Re: [PATCH] fuse: Mark fscrypt ioctls as unrestricted

[Chirantan Ekbote <chirantan@chromium.org>]

On Fri, Apr 24, 2020 at 12:38 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> [+Cc linux-fscrypt@vger.kernel.org]
>
> On Thu, Apr 23, 2020 at 04:47:06PM +0900, Chirantan Ekbote wrote:
> > The definitions for these 2 ioctls have been reversed: "get" is marked
> > as a write ioctl and "set" is marked as a read ioctl.  Moreover, since
> > these are now part of the public kernel interface they can never be
> > fixed because fixing them might break userspace applications compiled
> > with the older headers.
> >
> > Since the fuse module strictly enforces the ioctl encodings, it will
> > reject any attempt by the fuse server to correctly implement these
> > ioctls.  Instead, check if the process is trying to make one of these
> > ioctls and mark it unrestricted.  This will allow the server to fix the
> > encoding by reading/writing the correct data.
> >
> > Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
> > ---
> >  fs/fuse/file.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 9d67b830fb7a2..9b6d993323d53 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/swap.h>
> >  #include <linux/falloc.h>
> >  #include <linux/uio.h>
> > +#include <linux/fscrypt.h>
> >
> >  static struct page **fuse_pages_alloc(unsigned int npages, gfp_t flags,
> >                                     struct fuse_page_desc **desc)
> > @@ -2751,6 +2752,16 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
> >
> >       fuse_page_descs_length_init(ap.descs, 0, fc->max_pages);
> >
> > +     /*
> > +      * These commands are encoded backwards so it is literally impossible
> > +      * for a fuse server to implement them. Instead, mark them unrestricted
> > +      * so that the server can deal with the broken encoding itself.
> > +      */
> > +     if (cmd == FS_IOC_GET_ENCRYPTION_POLICY ||
> > +         cmd == FS_IOC_SET_ENCRYPTION_POLICY) {
> > +             flags |= FUSE_IOCTL_UNRESTRICTED;
> > +     }
>
> Are there any security concerns with marking these ioctls unrestricted, as
> opposed to dealing with the payload in the kernel?
>

The concern would be that the fuse server would be able to read/write
arbitrary memory in the calling process.  This isn't a concern for
something like virtiofs because the device already has access to all
of the VM's memory but it can be a concern with regular fuse servers.

> Also, can you elaborate on why you need only these two specific ioctls?
> FS_IOC_GET_ENCRYPTION_POLICY_EX and FS_IOC_ADD_ENCRYPTION_KEY take a
> variable-length payload and thus are similarly incompatible with FUSE, right?
> I thought we had discussed that for your use case the ioctl you actually need
> isn't the above two, but rather FS_IOC_GET_ENCRYPTION_POLICY_EX.  So I'm a bit
> confused by this patch.
>

It seems I have misunderstood the requirements.  Let's continue the
discussion off-list.

Chirantan