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