linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX
       [not found] <20201207040303.906100-1-chirantan@chromium.org>
@ 2020-12-07 18:01 ` Eric Biggers
  2020-12-08  9:38 ` [PATCH v2 0/2] fuse: fscrypt ioctl support Chirantan Ekbote
  1 sibling, 0 replies; 7+ messages in thread
From: Eric Biggers @ 2020-12-07 18:01 UTC (permalink / raw)
  To: Chirantan Ekbote
  Cc: Miklos Szeredi, linux-fsdevel, Dylan Reid, Suleiman Souhlal,
	fuse-devel, linux-fscrypt

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 0/2] fuse: fscrypt ioctl support
       [not found] <20201207040303.906100-1-chirantan@chromium.org>
  2020-12-07 18:01 ` [PATCH] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX Eric Biggers
@ 2020-12-08  9:38 ` Chirantan Ekbote
  2020-12-08  9:38   ` [PATCH v2 1/2] fuse: Move ioctl length calculation to a separate function Chirantan Ekbote
  2020-12-08  9:38   ` [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX Chirantan Ekbote
  1 sibling, 2 replies; 7+ messages in thread
From: Chirantan Ekbote @ 2020-12-08  9:38 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-fsdevel, Dylan Reid, Suleiman Souhlal, fuse-devel,
	Eric Biggers, linux-fscrypt, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/2] fuse: Move ioctl length calculation to a separate function
  2020-12-08  9:38 ` [PATCH v2 0/2] fuse: fscrypt ioctl support Chirantan Ekbote
@ 2020-12-08  9:38   ` Chirantan Ekbote
  2020-12-11 18:11     ` Eric Biggers
  2020-12-08  9:38   ` [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX Chirantan Ekbote
  1 sibling, 1 reply; 7+ messages in thread
From: Chirantan Ekbote @ 2020-12-08  9:38 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-fsdevel, Dylan Reid, Suleiman Souhlal, fuse-devel,
	Eric Biggers, linux-fscrypt, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX
  2020-12-08  9:38 ` [PATCH v2 0/2] fuse: fscrypt ioctl support Chirantan Ekbote
  2020-12-08  9:38   ` [PATCH v2 1/2] fuse: Move ioctl length calculation to a separate function Chirantan Ekbote
@ 2020-12-08  9:38   ` Chirantan Ekbote
  2020-12-11 18:12     ` Eric Biggers
  2021-02-15 14:49     ` Miklos Szeredi
  1 sibling, 2 replies; 7+ messages in thread
From: Chirantan Ekbote @ 2020-12-08  9:38 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-fsdevel, Dylan Reid, Suleiman Souhlal, fuse-devel,
	Eric Biggers, linux-fscrypt, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] fuse: Move ioctl length calculation to a separate function
  2020-12-08  9:38   ` [PATCH v2 1/2] fuse: Move ioctl length calculation to a separate function Chirantan Ekbote
@ 2020-12-11 18:11     ` Eric Biggers
  0 siblings, 0 replies; 7+ messages in thread
From: Eric Biggers @ 2020-12-11 18:11 UTC (permalink / raw)
  To: Chirantan Ekbote
  Cc: Miklos Szeredi, linux-fsdevel, Dylan Reid, Suleiman Souhlal,
	fuse-devel, linux-fscrypt

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX
  2020-12-08  9:38   ` [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX Chirantan Ekbote
@ 2020-12-11 18:12     ` Eric Biggers
  2021-02-15 14:49     ` Miklos Szeredi
  1 sibling, 0 replies; 7+ messages in thread
From: Eric Biggers @ 2020-12-11 18:12 UTC (permalink / raw)
  To: Chirantan Ekbote
  Cc: Miklos Szeredi, linux-fsdevel, Dylan Reid, Suleiman Souhlal,
	fuse-devel, linux-fscrypt

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX
  2020-12-08  9:38   ` [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX Chirantan Ekbote
  2020-12-11 18:12     ` Eric Biggers
@ 2021-02-15 14:49     ` Miklos Szeredi
  1 sibling, 0 replies; 7+ messages in thread
From: Miklos Szeredi @ 2021-02-15 14:49 UTC (permalink / raw)
  To: Chirantan Ekbote
  Cc: linux-fsdevel, Dylan Reid, Suleiman Souhlal, fuse-devel,
	Eric Biggers, linux-fscrypt

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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-02-15 14:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20201207040303.906100-1-chirantan@chromium.org>
2020-12-07 18:01 ` [PATCH] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX Eric Biggers
2020-12-08  9:38 ` [PATCH v2 0/2] fuse: fscrypt ioctl support Chirantan Ekbote
2020-12-08  9:38   ` [PATCH v2 1/2] fuse: Move ioctl length calculation to a separate function Chirantan Ekbote
2020-12-11 18:11     ` Eric Biggers
2020-12-08  9:38   ` [PATCH v2 2/2] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX Chirantan Ekbote
2020-12-11 18:12     ` Eric Biggers
2021-02-15 14:49     ` Miklos Szeredi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).