* 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 related [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
* [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 related [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).