| Message ID | 20201207040303.906100-1-chirantan@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX | expand |
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
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(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index c03034e8c1529..1627c14e9dacc 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) @@ -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: { + struct fscrypt_get_policy_ex_arg policy; + unsigned long size_ptr = + arg + offsetof(struct fscrypt_get_policy_ex_arg, + policy_size); + + 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; + } default: iov->iov_len = _IOC_SIZE(cmd); break;
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> --- fs/fuse/file.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)