On Mon, Oct 23, 2023 at 06:55:56PM +0100, Joao Martins wrote:
> On 23/10/2023 17:34, Jason Gunthorpe wrote:
> > On Mon, Oct 23, 2023 at 05:31:22PM +0100, Joao Martins wrote:
> >>> 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.
> >
> > That makes sense, much clearer what it is trying to do that way
>
> In regards to clearity, I am still checking if bitmap::page_size being 0 or not,
> to avoid being so implicitly in the last check i.e. (bitmap->iova &
> (bitmap->page_size - 1)) being an and to 0xfffffffff.
I think you have to check explicitly, iova/last_iova could also be
zero..
Jason
