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; }