| Message ID | 20240416001639.359059-1-richardfung@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] fuse: Add initial support for fs-verity | expand |
Hi, On Tue, Apr 16, 2024 at 12:16:39AM +0000, Richard Fung wrote: > This adds support for the FS_IOC_ENABLE_VERITY and FS_IOC_MEASURE_VERITY > ioctls. The FS_IOC_READ_VERITY_METADATA is missing but from the > documentation, "This is a fairly specialized use case, and most fs-verity > users won’t need this ioctl." > > Signed-off-by: Richard Fung <richardfung@google.com> > --- > fs/fuse/ioctl.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c > index 726640fa439e..01638784972a 100644 > --- a/fs/fuse/ioctl.c > +++ b/fs/fuse/ioctl.c > @@ -8,6 +8,7 @@ > #include <linux/uio.h> > #include <linux/compat.h> > #include <linux/fileattr.h> > +#include <linux/fsverity.h> > > static ssize_t fuse_send_ioctl(struct fuse_mount *fm, struct fuse_args *args, > struct fuse_ioctl_out *outarg) > @@ -118,6 +119,63 @@ static int fuse_copy_ioctl_iovec(struct fuse_conn *fc, struct iovec *dst, > } > > > +/* For fs-verity, determine iov lengths from input */ > +static long fuse_setup_verity_ioctl(unsigned int cmd, unsigned long arg, > + struct iovec *iov, unsigned int *in_iovs) > +{ > + switch (cmd) { > + case FS_IOC_MEASURE_VERITY: { > + __u16 digest_size; > + struct fsverity_digest __user *uarg = > + (struct fsverity_digest __user *)arg; > + > + if (copy_from_user(&digest_size, &uarg->digest_size, > + sizeof(digest_size))) > + return -EFAULT; > + > + if (digest_size > SIZE_MAX - sizeof(struct fsverity_digest)) > + return -EINVAL; > + > + iov->iov_len = sizeof(struct fsverity_digest) + digest_size; > + break; > + } > + case FS_IOC_ENABLE_VERITY: { > + struct fsverity_enable_arg enable; > + struct fsverity_enable_arg __user *uarg = > + (struct fsverity_enable_arg __user *)arg; > + const __u32 max_buffer_len = FUSE_MAX_MAX_PAGES * PAGE_SIZE; > + > + if (copy_from_user(&enable, uarg, sizeof(enable))) > + return -EFAULT; > + > + if (enable.salt_size > max_buffer_len || > + enable.sig_size > max_buffer_len) > + return -ENOMEM; > + > + if (enable.salt_size > 0) { > + iov++; > + (*in_iovs)++; > + > + iov->iov_base = u64_to_user_ptr(enable.salt_ptr); > + iov->iov_len = enable.salt_size; > + } > + > + if (enable.sig_size > 0) { > + iov++; > + (*in_iovs)++; > + > + iov->iov_base = u64_to_user_ptr(enable.sig_ptr); > + iov->iov_len = enable.sig_size; > + } > + break; > + } > + default: > + break; > + } > + return 0; > +} > + > + > /* > * For ioctls, there is no generic way to determine how much memory > * needs to be read and/or written. Furthermore, ioctls are allowed > @@ -227,6 +285,12 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg, > out_iov = iov; > out_iovs = 1; > } > + > + if (cmd == FS_IOC_MEASURE_VERITY || cmd == FS_IOC_ENABLE_VERITY) { > + err = fuse_setup_verity_ioctl(cmd, arg, iov, &in_iovs); > + if (err) > + goto out; > + } This looks like it passes on the correct buffers for these two ioctls, so if the FUSE developers agree that this works and is secure, consider this acked: Acked-by: Eric Biggers <ebiggers@google.com> It's a bit awkward that the ioctl number is checked twice, though. Maybe rename the new function to fuse_setup_special_ioctl() and call it unconditionally? - Eric
On Fri, Apr 19, 2024 at 10:05 AM Eric Biggers <ebiggers@kernel.org> wrote: > It's a bit awkward that the ioctl number is checked twice, though. Maybe rename > the new function to fuse_setup_special_ioctl() and call it unconditionally? > > - Eric I'm happy to change it but the benefit of the current iteration is that it's much more obvious to someone unfamiliar with the code that code path is only used for fs-verity. Admittedly, I may be overindexing on the importance of that as someone who's trying to learn the codebase myself. Let me know if you still prefer I change it
On Mon, 22 Apr 2024 at 18:31, Richard Fung <richardfung@google.com> wrote: > > On Fri, Apr 19, 2024 at 10:05 AM Eric Biggers <ebiggers@kernel.org> wrote: > > It's a bit awkward that the ioctl number is checked twice, though. Maybe rename > > the new function to fuse_setup_special_ioctl() and call it unconditionally? > > > > - Eric > > I'm happy to change it but the benefit of the current iteration is > that it's much more obvious to someone unfamiliar with the code that > code path is only used for fs-verity. Admittedly, I may be > overindexing on the importance of that as someone who's trying to > learn the codebase myself. > > Let me know if you still prefer I change it Or move the switch out to the caller and split fuse_setup_verity_ioctl() into two separate functions, since there's no commonality between them (except that both are verity related). I did that transformation and pushed it to git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next Please verify that I didn't mess anything up. Thanks, Miklos
> Please verify that I didn't mess anything up.
Tested it and it works! Thanks for updating the patch
diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c index 726640fa439e..01638784972a 100644 --- a/fs/fuse/ioctl.c +++ b/fs/fuse/ioctl.c @@ -8,6 +8,7 @@ #include <linux/uio.h> #include <linux/compat.h> #include <linux/fileattr.h> +#include <linux/fsverity.h> static ssize_t fuse_send_ioctl(struct fuse_mount *fm, struct fuse_args *args, struct fuse_ioctl_out *outarg) @@ -118,6 +119,63 @@ static int fuse_copy_ioctl_iovec(struct fuse_conn *fc, struct iovec *dst, } +/* For fs-verity, determine iov lengths from input */ +static long fuse_setup_verity_ioctl(unsigned int cmd, unsigned long arg, + struct iovec *iov, unsigned int *in_iovs) +{ + switch (cmd) { + case FS_IOC_MEASURE_VERITY: { + __u16 digest_size; + struct fsverity_digest __user *uarg = + (struct fsverity_digest __user *)arg; + + if (copy_from_user(&digest_size, &uarg->digest_size, + sizeof(digest_size))) + return -EFAULT; + + if (digest_size > SIZE_MAX - sizeof(struct fsverity_digest)) + return -EINVAL; + + iov->iov_len = sizeof(struct fsverity_digest) + digest_size; + break; + } + case FS_IOC_ENABLE_VERITY: { + struct fsverity_enable_arg enable; + struct fsverity_enable_arg __user *uarg = + (struct fsverity_enable_arg __user *)arg; + const __u32 max_buffer_len = FUSE_MAX_MAX_PAGES * PAGE_SIZE; + + if (copy_from_user(&enable, uarg, sizeof(enable))) + return -EFAULT; + + if (enable.salt_size > max_buffer_len || + enable.sig_size > max_buffer_len) + return -ENOMEM; + + if (enable.salt_size > 0) { + iov++; + (*in_iovs)++; + + iov->iov_base = u64_to_user_ptr(enable.salt_ptr); + iov->iov_len = enable.salt_size; + } + + if (enable.sig_size > 0) { + iov++; + (*in_iovs)++; + + iov->iov_base = u64_to_user_ptr(enable.sig_ptr); + iov->iov_len = enable.sig_size; + } + break; + } + default: + break; + } + return 0; +} + + /* * For ioctls, there is no generic way to determine how much memory * needs to be read and/or written. Furthermore, ioctls are allowed @@ -227,6 +285,12 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg, out_iov = iov; out_iovs = 1; } + + if (cmd == FS_IOC_MEASURE_VERITY || cmd == FS_IOC_ENABLE_VERITY) { + err = fuse_setup_verity_ioctl(cmd, arg, iov, &in_iovs); + if (err) + goto out; + } } retry:
This adds support for the FS_IOC_ENABLE_VERITY and FS_IOC_MEASURE_VERITY ioctls. The FS_IOC_READ_VERITY_METADATA is missing but from the documentation, "This is a fairly specialized use case, and most fs-verity users won’t need this ioctl." Signed-off-by: Richard Fung <richardfung@google.com> --- fs/fuse/ioctl.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)