On Tue, Aug 08, 2023 at 09:34:03AM -0300, Jason Gunthorpe wrote: > On Mon, Aug 07, 2023 at 08:12:37PM -0700, Nicolin Chen wrote: > > On Mon, Aug 07, 2023 at 03:08:29PM +0000, Liu, Yi L wrote: > > > > > From: Liu, Yi L <yi.l.liu@xxxxxxxxx> > > > > > Sent: Monday, July 24, 2023 7:14 PM > > > > > > > > > > +static int intel_nested_cache_invalidate_user(struct iommu_domain > > > > > *domain, > > > > > + void *user_data) > > > > > +{ > > > > > + struct iommu_hwpt_vtd_s1_invalidate_desc *req = user_data; > > > > > + struct iommu_hwpt_vtd_s1_invalidate *inv_info = user_data; > > > > > + struct dmar_domain *dmar_domain = to_dmar_domain(domain); > > > > > + unsigned int entry_size = inv_info->entry_size; > > > > > + u64 uptr = inv_info->inv_data_uptr; > > > > > + u64 nr_uptr = inv_info->entry_nr_uptr; > > > > > + struct device_domain_info *info; > > > > > + u32 entry_nr, index; > > > > > + unsigned long flags; > > > > > + int ret = 0; > > > > > + > > > > > + if (get_user(entry_nr, (uint32_t __user *)u64_to_user_ptr(nr_uptr))) > > > > > + return -EFAULT; > > > > > + > > > > > + for (index = 0; index < entry_nr; index++) { > > > > > + ret = copy_struct_from_user(req, sizeof(*req), > > > > > + u64_to_user_ptr(uptr + index * > > > > > entry_size), > > > > > + entry_size); > > > > > > > > If continuing this direction then the driver should also check minsz etc. > > > > for struct iommu_hwpt_vtd_s1_invalidate and iommu_hwpt_vtd_s1_invalidate_desc > > > > since they are uAPI and subject to change. > > > > > > Then needs to define size in the uapi data structure, and copy size first and > > > check minsz before going forward. How about the structures for hwpt alloc > > > like struct iommu_hwpt_vtd_s1? Should check minsz for them as well? > > > > Assuming that every uAPI data structure needs a min_size, we can > > either add a structure holding all min_sizes like iommufd main.c > > or have another xx_min_len in iommu_/domain_ops. > > If driver is doing the copy it is OK that driver does the min_size > check too Ah, just realized my reply above was missing a context.. Yi and I are having a concern that the core iommu_hpwt_alloc() and iommu_hwpt_cache_invalidate(), in the nesting series, copy data without checking the min_sizes. So, we are trying to add the missing piece into the next version but not sure which way could be optimal. It probably makes sense to add cache_invalidate_user_min_len next to the existing cache_invalidate_user_data_len. For the iommu_hwpt_alloc, we are missing a data_len, as the core just uses sizeof(union iommu_domain_user_data) when calling the copy_struct_from_user(). Perhaps we could add two pairs of data_len/min_len in the ops structs: // iommu_ops const size_t domain_alloc_user_data_len; // for sanity© const size_t domain_alloc_user_min_len; // for sanity only // iommu_domain_ops const size_t cache_invalidate_user_data_len; // for sanity© const size_t cache_invalidate_user_min_len; // for sanity only Thanks Nic