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

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

[Eric Biggers]

[+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?

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.

- Eric

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

[Chirantan Ekbote]

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