On 23/10/2023 17:16, Jason Gunthorpe wrote: > On Mon, Oct 23, 2023 at 04:56:01PM +0100, Joao Martins wrote: >> On 23/10/2023 13:41, Arnd Bergmann wrote: >>> On Mon, Oct 23, 2023, at 14:10, Jason Gunthorpe wrote: >>>> On Mon, Oct 23, 2023 at 10:28:13AM +0100, Joao Martins wrote: >>>>>> so it's probably >>>>>> best to add a range check plus type cast, rather than an >>>>>> expensive div_u64() here. >>>>> >>>>> OK >>>> >>>> Just keep it simple, we don't need to optimize for 32 bit. div_u64 >>>> will make the compiler happy. >>> >>> Fair enough. FWIW, I tried adding just the range check to see >>> if that would make the compiler turn it into a 32-bit division, >>> but that didn't work. >>> >>> Some type of range check might still be good to have for >>> unrelated reasons. >> >> I can reproduce the arm32 build problem and I'm applying this diff below to this >> patch to fix it. It essentially moves all the checks to >> iommufd_check_iova_range(), including range-check and adding div_u64. >> >> Additionally, perhaps should also move the iommufd_check_iova_range() invocation >> via io_pagetable.c code rather than hw-pagetable code? It seems to make more >> sense as there's nothing hw-pagetable specific that needs to be in here. > > Don't you need the IOAS though? > No, for these checks I only need the iopt, which I already pass into iopt_read_and_clear_dirty_data(). Everything can really be placed in the later. > Write it like this: > > int iommufd_check_iova_range(struct iommufd_ioas *ioas, > struct iommu_hwpt_get_dirty_bitmap *bitmap) > { > size_t iommu_pgsize = ioas->iopt.iova_alignment; > u64 last_iova; > > if (check_add_overflow(bitmap->iova, bitmap->length - 1, &last_iova)) > return -EOVERFLOW; > > if (bitmap->iova > ULONG_MAX || last_iova > ULONG_MAX) > return -EOVERFLOW; > > if ((bitmap->iova & (iommu_pgsize - 1)) || > ((last_iova + 1) & (iommu_pgsize - 1))) > return -EINVAL; > return 0; > } > > And if 0 should really be rejected then check iova == last_iova It should; Perhaps extending the above and replicate that second the ::page_size alignment check is important as it's what's used by the bitmap e.g. int iommufd_check_iova_range(struct iommufd_ioas *ioas, struct iommu_hwpt_get_dirty_bitmap *bitmap) { size_t iommu_pgsize = ioas->iopt.iova_alignment; u64 last_iova; if (check_add_overflow(bitmap->iova, bitmap->length - 1, &last_iova)) return -EOVERFLOW; if (bitmap->iova > ULONG_MAX || last_iova > ULONG_MAX) return -EOVERFLOW; if ((bitmap->iova & (iommu_pgsize - 1)) || ((last_iova + 1) & (iommu_pgsize - 1))) return -EINVAL; if ((bitmap->iova & (bitmap->page_size - 1)) || ((last_iova + 1) & (bitmap->page_size - 1))) return -EINVAL; return 0; }