| Message ID | 20240328205822.1007338-2-richardfung@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: Add initial support for fs-verity | expand |
Realized I should have added fuse-devel@ Could you please have a look? On Thu, Mar 28, 2024 at 1:58 PM Richard Fung <richardfung@google.com> 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 | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c > index 726640fa439e..a0e86c3de48f 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) > @@ -227,6 +228,57 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg, > out_iov = iov; > out_iovs = 1; > } > + > + /* For fs-verity, determine iov lengths from input */ > + 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; > + } > } > > retry: > -- > 2.44.0.478.gd926399ef9-goog >
On Thu, 28 Mar 2024 at 21:58, Richard Fung <richardfung@google.com> 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 | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c > index 726640fa439e..a0e86c3de48f 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) > @@ -227,6 +228,57 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg, > out_iov = iov; > out_iovs = 1; > } > + > + /* For fs-verity, determine iov lengths from input */ > + 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; > + } > } > > retry: I'm not thrilled by having ioctl specific handling added to the generic fuse ioctl code. But more important is what the fsverity folks think (CC's added). Thanks, Miklos
On Tue, Apr 09, 2024 at 04:50:10PM +0200, Miklos Szeredi wrote: > On Thu, 28 Mar 2024 at 21:58, Richard Fung <richardfung@google.com> 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 | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 52 insertions(+) > > > > diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c > > index 726640fa439e..a0e86c3de48f 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) > > @@ -227,6 +228,57 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg, > > out_iov = iov; > > out_iovs = 1; > > } > > + > > + /* For fs-verity, determine iov lengths from input */ > > + 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; > > + } > > } > > > > retry: > > I'm not thrilled by having ioctl specific handling added to the > generic fuse ioctl code. > > But more important is what the fsverity folks think (CC's added). > I am fine with having FUSE support passing through FS_IOC_MEASURE_VERITY and FS_IOC_ENABLE_VERITY. As you may have noticed, these ioctls are a bit more complex than the simple ones that FUSE allows already. The argument to FS_IOC_MEASURE_VERITY has a variable-length trailing array, and the argument to FS_IOC_ENABLE_VERITY has up to two pointers to other buffers. I am hoping the FUSE folks have thoughts on what is the best way to support ioctls like these. I suspect that this patch (with the special handling in FUSE) may be the only feasible approach, but I haven't properly investigated it. - Eric
On Wed, 10 Apr 2024 at 01:50, Eric Biggers <ebiggers@kernel.org> wrote: > I am fine with having FUSE support passing through FS_IOC_MEASURE_VERITY and > FS_IOC_ENABLE_VERITY. > > As you may have noticed, these ioctls are a bit more complex than the simple > ones that FUSE allows already. The argument to FS_IOC_MEASURE_VERITY has a > variable-length trailing array, and the argument to FS_IOC_ENABLE_VERITY has up > to two pointers to other buffers. > > I am hoping the FUSE folks have thoughts on what is the best way to support > ioctls like these. I suspect that this patch (with the special handling in > FUSE) may be the only feasible approach, but I haven't properly investigated it. Ideally I'd imagine something something similar to how we handle FS_IOC_GETFLAGS/SETFLAGS. Exceptions for those were also added in commit 31070f6ccec0 ("fuse: Fix parameter for FS_IOC_{GET,SET}FLAGS"). But then infrastructure was added to the vfs (commit 4c5b47997521 ("vfs: add fileattr ops")) so that filesystems can handle these as normal callbacks instead of dealing with ioctls directly. In the fsverity case this is not such a clear cut case, since only fuse (and possible network fs?) would actually implement the vfs callback, others would just set the default handler from fsverity. So I don't insist on doing this, just saying that it would be the cleanest outcome. If we do add exceptions, the requirement from me is that it's split out into a separate function from fuse_do_ioctl(). Thanks, Miklos
On Wed, Apr 10, 2024 at 11:06 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > Ideally I'd imagine something something similar to how we handle > FS_IOC_GETFLAGS/SETFLAGS. > > Exceptions for those were also added in commit 31070f6ccec0 ("fuse: > Fix parameter for FS_IOC_{GET,SET}FLAGS"). But then infrastructure > was added to the vfs (commit 4c5b47997521 ("vfs: add fileattr ops")) > so that filesystems can handle these as normal callbacks instead of > dealing with ioctls directly. > > In the fsverity case this is not such a clear cut case, since only > fuse (and possible network fs?) would actually implement the vfs > callback, others would just set the default handler from fsverity. So > I don't insist on doing this, just saying that it would be the > cleanest outcome. > > If we do add exceptions, the requirement from me is that it's split > out into a separate function from fuse_do_ioctl(). > > Thanks, > Miklos Thank you all for the feedback and suggestions! Would allowing FUSE_IOCTL_RETRY for these specific ioctls be possible/preferable? From my limited understanding retrying is designed to handle dynamically sized data. However it seems like that's currently only allowed for CUSE. If that's not a good idea then I'll try to split it into a separate function if you don't feel strongly about the other approach.
On Thu, 11 Apr 2024 at 21:16, Richard Fung <richardfung@google.com> wrote: > Would allowing FUSE_IOCTL_RETRY for these specific ioctls be > possible/preferable? From my limited understanding retrying is > designed to handle dynamically sized data. However it seems like > that's currently only allowed for CUSE. > > If that's not a good idea then I'll try to split it into a separate > function if you don't feel strongly about the other approach. It's not a good idea, because it gives complete freedom to the fuse server about where to gather data from, and that's just an invitation to malicious behavior. So yes, please split the current one off to a separate function and let's see how that looks. Thanks, Miklos
diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c index 726640fa439e..a0e86c3de48f 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) @@ -227,6 +228,57 @@ long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg, out_iov = iov; out_iovs = 1; } + + /* For fs-verity, determine iov lengths from input */ + 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; + } } 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 | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)