On Fri, Apr 24, 2020 at 12:38 AM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> [+Cc linux-fscrypt@xxxxxxxxxxxxxxx]
>
> 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@xxxxxxxxxxxx>
> > ---
> > 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
|