Re: [PATCH] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX

Re: [PATCH] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX

[Eric Biggers]

Please Cc linux-fscrypt@vger.kernel.org on all fscrypt-related patches.

On Mon, Dec 07, 2020 at 01:03:03PM +0900, Chirantan Ekbote wrote:
> This is a dynamically sized ioctl so we need to check the user-provided
> parameter for the actual length.
> 
> Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>

Could you add something here about why this ioctl in particular needs to be
passed through FUSE?  This isn't the only dynamically-sized ioctl.

> @@ -2808,6 +2809,21 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
>  		case FS_IOC_SETFLAGS:
>  			iov->iov_len = sizeof(int);
>  			break;
> +		case FS_IOC_GET_ENCRYPTION_POLICY_EX: {

This is in the middle of a 200 lines function.  It would be easier to understand
if you refactored this to use a helper function that that takes in the ioctl
number and user buffer and returns the size.

> +			struct fscrypt_get_policy_ex_arg policy;

'__u64 policy_size' would be sufficient, since only that part of the struct is
used.

> +			unsigned long size_ptr =
> +				arg + offsetof(struct fscrypt_get_policy_ex_arg,
> +					       policy_size);

Doing pointer arithmetic on unsigned long is unusual.  It would be easier to
understand if you did:

	struct fscrypt_get_policy_ex_arg __user *uarg =
		(struct fscrypt_get_policy_ex_arg __user *)arg;

Then pass &uarg->policy_size to copy_from_user().

> +
> +			if (copy_from_user(&policy.policy_size,
> +					   (void __user *)size_ptr,
> +					   sizeof(policy.policy_size)))
> +				return -EFAULT;
> +
> +			iov->iov_len =
> +				sizeof(policy.policy_size) + policy.policy_size;
> +			break;

This may overflow SIZE_MAX, as policy_size is a __u64 directly from userspace.
Wouldn't FUSE need to limit the size to a smaller value?

- Eric

[PATCH v2 0/2] fuse: fscrypt ioctl support

[Chirantan Ekbote]

This series adds support for the FS_IOC_GET_ENCRYPTION_POLICY_EX ioctl
to fuse.  We want this in Chrome OS because have applications running
inside a VM that expect this ioctl to succeed on a virtiofs mount.

This series doesn't add support for other dynamically-sized ioctls
because there don't appear to be any users for them.  However, once
these patches are merged it should hopefully be much simpler to add
support for other ioctls in the future, if necessary.

Changes in v2:
 * Move ioctl length calculation to a separate function.
 * Properly clean up in the error case.
 * Check that the user-provided size does not cause an integer
   overflow.

Chirantan Ekbote (2):
  fuse: Move ioctl length calculation to a separate function
  fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX

 fs/fuse/file.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v2 1/2] fuse: Move ioctl length calculation to a separate function

[Chirantan Ekbote]

This will make it more readable when we add support for more ioctls.

Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
---
 fs/fuse/file.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index c03034e8c1529..69cffb77a0b25 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2703,6 +2703,20 @@ static int fuse_copy_ioctl_iovec(struct fuse_conn *fc, struct iovec *dst,
 	return 0;
 }
 
+static int fuse_get_ioctl_len(unsigned int cmd, unsigned long arg, size_t *len)
+{
+	switch (cmd) {
+	case FS_IOC_GETFLAGS:
+	case FS_IOC_SETFLAGS:
+		*len = sizeof(int);
+		break;
+	default:
+		*len = _IOC_SIZE(cmd);
+		break;
+	}
+
+	return 0;
+}
 
 /*
  * For ioctls, there is no generic way to determine how much memory
@@ -2802,16 +2816,9 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
 		struct iovec *iov = iov_page;
 
 		iov->iov_base = (void __user *)arg;
-
-		switch (cmd) {
-		case FS_IOC_GETFLAGS:
-		case FS_IOC_SETFLAGS:
-			iov->iov_len = sizeof(int);
-			break;
-		default:
-			iov->iov_len = _IOC_SIZE(cmd);
-			break;
-		}
+		err = fuse_get_ioctl_len(cmd, arg, &iov->iov_len);
+		if (err)
+			goto out;
 
 		if (_IOC_DIR(cmd) & _IOC_WRITE) {
 			in_iov = iov;
-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX

[Chirantan Ekbote]

Chrome OS would like to support this ioctl when passed through the fuse
driver. However since it is dynamically sized, we can't rely on the
length encoded in the command.  Instead check the `policy_size` field of
the user provided parameter to get the max length of the data returned
by the server.

Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
---
 fs/fuse/file.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 69cffb77a0b25..b64ff7f2fe4dd 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -19,6 +19,7 @@
 #include <linux/falloc.h>
 #include <linux/uio.h>
 #include <linux/fs.h>
+#include <linux/fscrypt.h>
 
 static struct page **fuse_pages_alloc(unsigned int npages, gfp_t flags,
 				      struct fuse_page_desc **desc)
@@ -2710,6 +2711,21 @@ static int fuse_get_ioctl_len(unsigned int cmd, unsigned long arg, size_t *len)
 	case FS_IOC_SETFLAGS:
 		*len = sizeof(int);
 		break;
+	case FS_IOC_GET_ENCRYPTION_POLICY_EX: {
+		__u64 policy_size;
+		struct fscrypt_get_policy_ex_arg __user *uarg =
+			(struct fscrypt_get_policy_ex_arg __user *)arg;
+
+		if (copy_from_user(&policy_size, &uarg->policy_size,
+				   sizeof(policy_size)))
+			return -EFAULT;
+
+		if (policy_size > SIZE_MAX - sizeof(policy_size))
+			return -EINVAL;
+
+		*len = sizeof(policy_size) + policy_size;
+		break;
+	}
 	default:
 		*len = _IOC_SIZE(cmd);
 		break;
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v2 1/2] fuse: Move ioctl length calculation to a separate function

[Eric Biggers]

On Tue, Dec 08, 2020 at 06:38:07PM +0900, Chirantan Ekbote wrote:
> This will make it more readable when we add support for more ioctls.
> 
> Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>

Reviewed-by: Eric Biggers <ebiggers@google.com>

Re: [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX

[Eric Biggers]

On Tue, Dec 08, 2020 at 06:38:08PM +0900, Chirantan Ekbote wrote:
> Chrome OS would like to support this ioctl when passed through the fuse
> driver. However since it is dynamically sized, we can't rely on the
> length encoded in the command.  Instead check the `policy_size` field of
> the user provided parameter to get the max length of the data returned
> by the server.
> 
> Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>

Reviewed-by: Eric Biggers <ebiggers@google.com>

Re: [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX

[Miklos Szeredi]

On Tue, Dec 8, 2020 at 10:38 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
>
> Chrome OS would like to support this ioctl when passed through the fuse
> driver. However since it is dynamically sized, we can't rely on the
> length encoded in the command.  Instead check the `policy_size` field of
> the user provided parameter to get the max length of the data returned
> by the server.

I'd also maximize the length at sizeof(union fscrypt_policy).  I.e.
virtiofs doesn't need to support higher level versions than the client
kernel supports.

Also, I'm thinking about whether it's safe to enable in plain fuse in
addition to virtiofs.  I don't see a reason for not doing so, but
maybe it makes sense to keep disabled until a use case comes up.

Thanks,
Miklos


>
> Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
> ---
>  fs/fuse/file.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 69cffb77a0b25..b64ff7f2fe4dd 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -19,6 +19,7 @@
>  #include <linux/falloc.h>
>  #include <linux/uio.h>
>  #include <linux/fs.h>
> +#include <linux/fscrypt.h>
>
>  static struct page **fuse_pages_alloc(unsigned int npages, gfp_t flags,
>                                       struct fuse_page_desc **desc)
> @@ -2710,6 +2711,21 @@ static int fuse_get_ioctl_len(unsigned int cmd, unsigned long arg, size_t *len)
>         case FS_IOC_SETFLAGS:
>                 *len = sizeof(int);
>                 break;
> +       case FS_IOC_GET_ENCRYPTION_POLICY_EX: {
> +               __u64 policy_size;
> +               struct fscrypt_get_policy_ex_arg __user *uarg =
> +                       (struct fscrypt_get_policy_ex_arg __user *)arg;
> +
> +               if (copy_from_user(&policy_size, &uarg->policy_size,
> +                                  sizeof(policy_size)))
> +                       return -EFAULT;
> +
> +               if (policy_size > SIZE_MAX - sizeof(policy_size))
> +                       return -EINVAL;
> +
> +               *len = sizeof(policy_size) + policy_size;
> +               break;
> +       }
>         default:
>                 *len = _IOC_SIZE(cmd);
>                 break;
> --
> 2.29.2.576.ga3fc446d84-goog
>