On 2024/10/31 01:51, Alex Williamson wrote:
On Wed, 30 Oct 2024 20:54:09 +0800 Yi Liu yi.l.liu@intel.com wrote:
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.
No, xend denotes the end of the structure we need to satisfy the flags that are requested by the user.
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.
New field xyz would require a new flag, VFIO_DEVICE_XYZ and we'd extend the above code as:
if (attach.argsz < minsz) return -EINVAL;
if (attach.flags & (~(VFIO_DEVICE_ATTACH_PASID | VFIO_DEVICE_XYZ))) return -EINVAL;
if (attach.flags & VFIO_DEVICE_ATTACH_PASID) xend = offsetofend(struct vfio_device_attach_iommufd_pt, pasid);
if (attach.flags & VFIO_DEVICE_XYZ) xend = offsetofend(struct vfio_device_attach_iommufd_pt, xyz);
if (xend) { if (attach.argsz < xend) return -EINVAL;
New userspace can provide argsz = offsetofend(, xyz), just as it could provide argsz = PAGE_SIZE now if it really wanted, but argsz > minsz is only required if the user sets any of these new flags. Therefore old userspace on new kernel continues to work.
got it. This should work. thanks.:)
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)This is an -EINVAL condition, xend tracks the flags the user has set. The user must provide a sufficient buffer for the flags they've set.
size = attach.argsz - minsz; else size = xend - minsz;This is the only correct copy size.
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 a user sets the ATTACH_PASID flag and argsz is less than offsetofend(struct, pasid), we need to return -EINVAL as indicated above. Thanks,
yep.
if (copy_from_user((void *)&attach + minsz, (void __user *)arg + minsz, xend - minsz)) return -EFAULT; }