Re: [PATCH] fuse: Support FS_IOC_GET_ENCRYPTION_POLICY_EX

[Eric Biggers <ebiggers@kernel.org>]

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