Re: [PATCH v2] fuse: Add initial support for fs-verity

From: Eric Biggers <ebiggers@kernel.org>
To: Richard Fung <richardfung@google.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
	linux-fsdevel@vger.kernel.org, fsverity@lists.linux.dev,
	ynaffit@google.com
Subject: Re: [PATCH v2] fuse: Add initial support for fs-verity
Date: Fri, 19 Apr 2024 10:05:11 -0700	[thread overview]
Message-ID: <20240419170511.GB1131@sol.localdomain> (raw)
In-Reply-To: <20240416001639.359059-1-richardfung@google.com>

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