Re: [PATCH v5 4/5] vfio: Add vfio_copy_user_data()

[Yi Liu <yi.l.liu@intel.com>]

On 2024/11/12 08:03, Alex Williamson wrote:
> On Fri,  8 Nov 2024 04:17:41 -0800
> Yi Liu <yi.l.liu@intel.com> wrote:
> 
>> This generalizes the logic of copying user data when the user struct
>> Have new fields introduced. The helpers can be used by the vfio uapis
>> that have the argsz and flags fields in the beginning 8 bytes.
>>
>> As an example, the vfio_device_{at|de}tach_iommufd_pt paths are updated
>> to use the helpers.
>>
>> The flags may be defined to mark a new field in the structure, reuse
>> reserved fields, or special handling of an existing field. The extended
>> size would differ for different flags. Each user API that wants to use
>> the generalized helpers should define an array to store the corresponding
>> extended sizes for each defined flag.
>>
>> For example, we start out with the below, minsz is 12.
>>
>>    struct vfio_foo_struct {
>>    	__u32   argsz;
>>    	__u32   flags;
>>    	__u32   pt_id;
>>    };
>>
>> And then here it becomes:
>>
>>    struct vfio_foo_struct {
>>    	__u32   argsz;
>>    	__u32   flags;
>>    #define VFIO_FOO_STRUCT_PASID   (1 << 0)
>>    	__u32   pt_id;
>>    	__u32   pasid;
>>    };
>>
>> The array is { 16 }.
>>
>> If the next flag is simply related to the processing of @pt_id and
>> doesn't require @pasid, then the extended size of the new flag is
>> 12. The array become { 16, 12 }
>>
>>    struct vfio_foo_struct {
>>    	__u32   argsz;
>>    	__u32   flags;
>>    #define VFIO_FOO_STRUCT_PASID   (1 << 0)
>>    #define VFIO_FOO_STRUCT_SPECICAL_PTID   (1 << 1)
>>    	__u32   pt_id;
>>    	__u32   pasid;
>>    };
>>
>> Similarly, rather than adding new field, we might have reused a previously
>> reserved field, for instance what if we already expanded the structure
>> as the below, array is already { 24 }.
>>
>>    struct vfio_foo_struct {
>>    	__u32   argsz;
>>    	__u32   flags;
>>    #define VFIO_FOO_STRUCT_XXX     (1 << 0)
>>    	__u32   pt_id;
>>    	__u32   reserved;
>>    	__u64   xxx;
>>    };
>>
>> If we then want to add @pasid, we might really prefer to take advantage
>> of that reserved field and the array becomes { 24, 16 }.
>>
>>    struct vfio_foo_struct {
>>    	__u32   argsz;
>>    	__u32   flags;
>>    #define VFIO_FOO_STRUCT_XXX     (1 << 0)
>>    #define VFIO_FOO_STRUCT_PASID   (1 << 1)
>>    	__u32   pt_id;
>>    	__u32   reserved;
> 
> I think this was supposed to be s/reserved/pasid/

you are right.

>>    	__u64   xxx;
>>    };
>>
>> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/vfio/device_cdev.c | 81 +++++++++++++-------------------------
>>   drivers/vfio/vfio.h        | 18 +++++++++
>>   drivers/vfio/vfio_main.c   | 55 ++++++++++++++++++++++++++
>>   3 files changed, 100 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
>> index 4519f482e212..35c7664b9a97 100644
>> --- a/drivers/vfio/device_cdev.c
>> +++ b/drivers/vfio/device_cdev.c
>> @@ -159,40 +159,33 @@ void vfio_df_unbind_iommufd(struct vfio_device_file *df)
>>   	vfio_device_unblock_group(device);
>>   }
>>   
>> +#define VFIO_ATTACH_FLAGS_MASK VFIO_DEVICE_ATTACH_PASID
>> +static unsigned long
>> +vfio_attach_xends[ilog2(VFIO_ATTACH_FLAGS_MASK) + 1] = {
>> +	XEND_SIZE(VFIO_DEVICE_ATTACH_PASID,
>> +		  struct vfio_device_attach_iommufd_pt, pasid),
>> +};
>> +
>> +#define VFIO_DETACH_FLAGS_MASK VFIO_DEVICE_DETACH_PASID
>> +static unsigned long
>> +vfio_detach_xends[ilog2(VFIO_DETACH_FLAGS_MASK) + 1] = {
>> +	XEND_SIZE(VFIO_DEVICE_DETACH_PASID,
>> +		  struct vfio_device_detach_iommufd_pt, pasid),
>> +};
>> +
>>   int vfio_df_ioctl_attach_pt(struct vfio_device_file *df,
>>   			    struct vfio_device_attach_iommufd_pt __user *arg)
>>   {
>>   	struct vfio_device_attach_iommufd_pt attach;
>>   	struct vfio_device *device = df->device;
>> -	unsigned long minsz, xend = 0;
>>   	int ret;
>>   
>> -	minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
>> -
>> -	if (copy_from_user(&attach, arg, minsz))
>> -		return -EFAULT;
>> -
>> -	if (attach.argsz < minsz)
>> -		return -EINVAL;
>> -
>> -	if (attach.flags & (~VFIO_DEVICE_ATTACH_PASID))
>> -		return -EINVAL;
>> -
>> -	if (attach.flags & VFIO_DEVICE_ATTACH_PASID)
>> -		xend = offsetofend(struct vfio_device_attach_iommufd_pt, pasid);
>> -
>> -	/*
>> -	 * xend may be equal to minsz if a flag is defined for reusing a
>> -	 * reserved field or a special usage of an existing field.
>> -	 */
>> -	if (xend > minsz) {
>> -		if (attach.argsz < xend)
>> -			return -EINVAL;
>> -
>> -		if (copy_from_user((void *)&attach + minsz,
>> -				   (void __user *)arg + minsz, xend - minsz))
>> -			return -EFAULT;
>> -	}
>> +	ret = vfio_copy_user_data((void __user *)arg, &attach,
>> +				  struct vfio_device_attach_iommufd_pt,
>> +				  pt_id, VFIO_ATTACH_FLAGS_MASK,
>> +				  vfio_attach_xends);
>> +	if (ret)
>> +		return ret;
>>   
>>   	if ((attach.flags & VFIO_DEVICE_ATTACH_PASID) &&
>>   	    !device->ops->pasid_attach_ioas)
>> @@ -227,34 +220,14 @@ int vfio_df_ioctl_detach_pt(struct vfio_device_file *df,
>>   {
>>   	struct vfio_device_detach_iommufd_pt detach;
>>   	struct vfio_device *device = df->device;
>> -	unsigned long minsz, xend = 0;
>> -
>> -	minsz = offsetofend(struct vfio_device_detach_iommufd_pt, flags);
>> -
>> -	if (copy_from_user(&detach, arg, minsz))
>> -		return -EFAULT;
>> -
>> -	if (detach.argsz < minsz)
>> -		return -EINVAL;
>> -
>> -	if (detach.flags & (~VFIO_DEVICE_DETACH_PASID))
>> -		return -EINVAL;
>> -
>> -	if (detach.flags & VFIO_DEVICE_DETACH_PASID)
>> -		xend = offsetofend(struct vfio_device_detach_iommufd_pt, pasid);
>> -
>> -	/*
>> -	 * xend may be equal to minsz if a flag is defined for reusing a
>> -	 * reserved field or a special usage of an existing field.
>> -	 */
>> -	if (xend > minsz) {
>> -		if (detach.argsz < xend)
>> -			return -EINVAL;
>> +	int ret;
>>   
>> -		if (copy_from_user((void *)&detach + minsz,
>> -				   (void __user *)arg + minsz, xend - minsz))
>> -			return -EFAULT;
>> -	}
>> +	ret = vfio_copy_user_data((void __user *)arg, &detach,
>> +				  struct vfio_device_detach_iommufd_pt,
>> +				  flags, VFIO_DETACH_FLAGS_MASK,
>> +				  vfio_detach_xends);
>> +	if (ret)
>> +		return ret;
>>   
>>   	if ((detach.flags & VFIO_DEVICE_DETACH_PASID) &&
>>   	    !device->ops->pasid_detach_ioas)
>> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
>> index 50128da18bca..87bed550c46e 100644
>> --- a/drivers/vfio/vfio.h
>> +++ b/drivers/vfio/vfio.h
>> @@ -34,6 +34,24 @@ void vfio_df_close(struct vfio_device_file *df);
>>   struct vfio_device_file *
>>   vfio_allocate_device_file(struct vfio_device *device);
>>   
>> +int vfio_copy_from_user(void *buffer, void __user *arg,
>> +			unsigned long minsz, u32 flags_mask,
>> +			unsigned long *xend_array);
>> +
>> +#define vfio_copy_user_data(_arg, _local_buffer, _struct, _min_last,          \
>> +			    _flags_mask, _xend_array)                         \
>> +	vfio_copy_from_user(_local_buffer, _arg,                              \
>> +			    offsetofend(_struct, _min_last) +                \
>> +			    BUILD_BUG_ON_ZERO(offsetof(_struct, argsz) !=     \
>> +					      0) +                            \
>> +			    BUILD_BUG_ON_ZERO(offsetof(_struct, flags) !=     \
>> +					      sizeof(u32)),                   \
>> +			    _flags_mask, _xend_array)
>> +
>> +#define XEND_SIZE(_flag, _struct, _xlast)                                    \
>> +	[ilog2(_flag)] = offsetofend(_struct, _xlast) +                      \
>> +			 BUILD_BUG_ON_ZERO(_flag == 0)                       \
>> +
>>   extern const struct file_operations vfio_device_fops;
>>   
>>   #ifdef CONFIG_VFIO_NOIOMMU
>> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
>> index a5a62d9d963f..c61336ea5123 100644
>> --- a/drivers/vfio/vfio_main.c
>> +++ b/drivers/vfio/vfio_main.c
>> @@ -1694,6 +1694,61 @@ int vfio_dma_rw(struct vfio_device *device, dma_addr_t iova, void *data,
>>   }
>>   EXPORT_SYMBOL(vfio_dma_rw);
>>   
>> +/**
>> + * vfio_copy_from_user - Copy the user struct that may have extended fields
>> + *
>> + * @buffer: The local buffer to store the data copied from user
>> + * @arg: The user buffer pointer
>> + * @minsz: The minimum size of the user struct
>> + * @flags_mask: The combination of all the falgs defined
>> + * @xend_array: The array that stores the xend size for set flags.
>> + *
>> + * This helper requires the user struct put the argsz and flags fields in
>> + * the first 8 bytes.
>> + *
>> + * Return 0 for success, otherwise -errno
>> + */
>> +int vfio_copy_from_user(void *buffer, void __user *arg,
> 
> This should probably be prefixed with an underscore and note that
> callers should use the wrapper function to impose the parameter
> checking.

got it.

> 
>> +			unsigned long minsz, u32 flags_mask,
>> +			unsigned long *xend_array)
>> +{
>> +	unsigned long xend = minsz;
>> +	struct user_header {
>> +		u32 argsz;
>> +		u32 flags;
>> +	} *header;
>> +	unsigned long flags;
>> +	u32 flag;
>> +
>> +	if (copy_from_user(buffer, arg, minsz))
>> +		return -EFAULT;
>> +
>> +	header = (struct user_header *)buffer;
>> +	if (header->argsz < minsz)
>> +		return -EINVAL;
>> +
>> +	if (header->flags & ~flags_mask)
>> +		return -EINVAL;
> 
> I'm already wrestling with whether this is an over engineered solution
> to remove a couple dozen lines of mostly duplicate logic between attach
> and detach, but a couple points that could make it more versatile:
> 
> (1) Test xend_array here:
> 
> 	if (!xend_array)
> 		return 0;

Perhaps we should return error if the header->flags has any bit set. Such
cases require a valid xend_array.

> 
> (2) Return ssize_t/-errno for the caller to know the resulting copy
> size.
> 
>> +
>> +	/* Loop each set flag to decide the xend */
>> +	flags = header->flags;
>> +	for_each_set_bit(flag, &flags, BITS_PER_TYPE(u32)) {
>> +		if (xend_array[flag] > xend)
>> +			xend = xend_array[flag];
> 
> Can we craft a BUILD_BUG in the wrapper to test that xend_array is at
> least long enough to match the highest bit in flags?  Thanks,

yes. I would add a BUILD_BUG like the below.

BUILD_BUG_ON(ARRAY_SIZE(_xend_array) < ilog2(_flags_mask));

-- 
Regards,
Yi Liu