Re: [PATCH v3 3/4] vfio: Add VFIO_DEVICE_PASID_[AT|DE]TACH_IOMMUFD_PT

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

Hi Alex,

On 2024/10/18 13:40, Yi Liu wrote:
>>>> I think we need to monotonically increase the structure size,
>>>> but maybe something more like below, using flags.  The expectation
>>>> would be that if we add another flag that extends the structure, we'd
>>>> test that flag after PASID and clobber xend to a new value further into
>>>> the new structure.  We'd also add that flag to the flags mask, but we'd
>>>> share the copy code.
>>>
>>> agree, this share code might be needed for other path as well. Some macros
>>> I guess.
>>>
>>>>
>>>>     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);
>>>>
>>>>     if (xend) {
>>>>         if (attach.argsz < xend)
>>>>             return -EINVAL;

Need to check the future usage of 'xend'. In understanding, 'xend' should
always be offsetofend(struct, the_last_field). A userspace that uses @pasid 
field would set argsz >= offsetofend(struct, pasid), most likely it would
just set argsz==offsetofend(struct, pasid). If so, such userspace would be
failed when running on a kernel that has added new fields behind @pasid.

Say two decades later, we add a new field (say @xyz) to this user struct,
the 'xend' would be updated to be offsetofend(struct, xyz). This 'xend'
would be larger than the argsz provided by the aforementioned userspace.
Hence it would be failed in the above check. To make it work, I'm
considering to make some changes to the code. When argsz < xend, we only
copy extra data with size==argsz-minsz. Just as the below.

	if (xend) {
		unsigned long size;

		if (attach.argsz < xend)
			size = attach.argsz - minsz;
		else
			size = xend - minsz;

		if (copy_from_user((void *)&attach + minsz,
				  (void __user *)arg + minsz, size))
			return -EFAULT;
	}

However, it seems to have another problem. If the userspace that uses
@pasid set the argsz==offsetofend(struct, pasid) - 1, such userspace is
not supposed to work and should be failed by kernel. is it? However, my
above code cannot fail it. :(

Any suggestion about it?

>>>>
>>>>         if (copy_from_user((void *)&attach + minsz,
>>>>                     (void __user *)arg + minsz, xend - minsz))
>>>>             return -EFAULT;
>>>>     }
>>>

-- 
Regards,
Yi Liu