> From: Jacob Pan <jacob.jun....@linux.intel.com> > Sent: Wednesday, April 15, 2020 6:32 AM > > On Tue, 14 Apr 2020 10:13:04 -0700 > Jacob Pan <jacob.jun....@linux.intel.com> wrote: > > > > > > In any of the proposed solutions, the > > > > > IOMMU driver is ultimately responsible for validating the user > > > > > data, so do we want vfio performing the copy_from_user() to an > > > > > object that could later be assumed to be sanitized, or should > > > > > vfio just pass a user pointer to make it obvious that the > > > > > consumer is responsible for all the user protections? Seems > > > > > like the latter. > > > > I like the latter as well. > > > > > On a second thought, I think the former is better. Two reasons: > > 1. IOMMU API such as page_response is also used in baremetal. So it is > not suitable to pass a __user *. > https://www.spinics.net/lists/arm-kernel/msg798677.html
You can have a wrapped version accepting a __user* and an internal version for kernel pointers. > > 2. Some data are in the mandatory (fixed offset, never removed or > extended) portion of the uAPI structure. It is simpler for VFIO to > extract that and pass it to IOMMU API. For example, the PASID value used > for unbind_gpasid(). VFIO also need to sanitize the PASID value to make > sure it belongs to the same VM that did the allocation. I don't think this makes much difference. If anyway you still plan to let IOMMU driver parse some user pointers, why not making a clear split to have it sparse all IOMMU specific fields? Thanks Kevin > > > > > > > That still really > > > > > doesn't address what's in that user data blob yet, but the vfio > > > > > interface could be: > > > > > > > > > > struct { > > > > > __u32 argsz; > > > > > __u32 flags; > > > > > __u8 data[]; > > > > > } > > > > > > > > > > Where flags might be partitioned like we do for DEVICE_FEATURE > > > > > to indicate the format of data and what vfio should do with it, > > > > > and data might simply be defined as a (__u64 __user *). > > > > > > > > > So, __user * will be passed to IOMMU driver if VFIO checks minsz > > > > include flags and they are valid. > > > > IOMMU driver can copy the rest based on the mandatory > > > > version/minsz and flags in the IOMMU uAPI structs. > > > > Does it sound right? This is really choice #2. > > > > > > Sounds like each IOMMU UAPI struct just needs to have an embedded > > > size and flags field, but yes. > > > > > Yes, an argsz field can be added to each UAPI. There are already flags > > or the equivalent. IOMMU driver can process the __user * based on the > > argsz, flags, check argsz against offsetofend(iommu_uapi_struct, > > last_element), etc.; _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu