Please Cc linux-fscrypt@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxx>
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
