On 23/10/2023 10:09, Arnd Bergmann wrote:
> On Sat, Oct 21, 2023, at 00:27, Joao Martins wrote:
>
>> +int iommufd_check_iova_range(struct iommufd_ioas *ioas,
>> + struct iommu_hwpt_get_dirty_bitmap *bitmap)
>> +{
>> + unsigned long npages;
>> + size_t iommu_pgsize;
>> + int rc = -EINVAL;
>> +
>> + if (!bitmap->page_size)
>> + return rc;
>> +
>> + npages = bitmap->length / bitmap->page_size;
>
>
> The 64-bit division causes a link failure on 32-bit architectures:
>
> arm-linux-gnueabi-ld: drivers/iommu/iommufd/hw_pagetable.o: in function `iommufd_check_iova_range':
> hw_pagetable.c:(.text+0x77e): undefined reference to `__aeabi_uldivmod'
> arm-linux-gnueabi/bin/arm-linux-gnueabi-ld: drivers/iommu/iommufd/hw_pagetable.o: in function `iommufd_hwpt_get_dirty_bitmap':
> hw_pagetable.c:(.text+0x84c): undefined reference to `__aeabi_uldivmod'
>
> I think it is safe to assume that the length of the bitmap
> does not overflow an 'unsigned long',
Also I do check for the overflow but comes later; I should move the overflow
check from iopt_read_and_clear_dirty_data() into here as it's the only entry
point of that function, while deleting the duplicated alignment check from
iopt_read_and_clear_dirty_data().
> so it's probably
> best to add a range check plus type cast, rather than an
> expensive div_u64() here.
>
OK
>> +/**
>> + * struct iommu_hwpt_get_dirty_bitmap - ioctl(IOMMU_HWPT_GET_DIRTY_BITMAP)
>> + * @size: sizeof(struct iommu_hwpt_get_dirty_bitmap)
>> + * @hwpt_id: HW pagetable ID that represents the IOMMU domain
>> + * @flags: Must be zero
>> + * @iova: base IOVA of the bitmap first bit
>> + * @length: IOVA range size
>> + * @page_size: page size granularity of each bit in the bitmap
>> + * @data: bitmap where to set the dirty bits. The bitmap bits each
>> + * represent a page_size which you deviate from an arbitrary iova.
>> + *
>> + * Checking a given IOVA is dirty:
>> + *
>> + * data[(iova / page_size) / 64] & (1ULL << ((iova / page_size) % 64))
>> + *
>> + * Walk the IOMMU pagetables for a given IOVA range to return a bitmap
>> + * with the dirty IOVAs. In doing so it will also by default clear any
>> + * dirty bit metadata set in the IOPTE.
>> + */
>> +struct iommu_hwpt_get_dirty_bitmap {
>> + __u32 size;
>> + __u32 hwpt_id;
>> + __u32 flags;
>> + __u32 __reserved;
>> + __aligned_u64 iova;
>> + __aligned_u64 length;
>> + __aligned_u64 page_size;
>> + __aligned_u64 *data;
>> +};
>> +#define IOMMU_HWPT_GET_DIRTY_BITMAP _IO(IOMMUFD_TYPE, \
>> + IOMMUFD_CMD_HWPT_GET_DIRTY_BITMAP)
>> +
>
> This is a flawed definition for an ioctl data structure. While
> it appears that you have tried hard to follow the recommendations
> in Documentation/driver-api/ioctl.rst, you accidentally added
> a pointer here, which breaks compat mode handling because of
> the uninitialized padding after the 32-bit 'data' pointer.
>
Right
> The correct fix here is to use a __u64 value for the pointer
> itself and convert it like:
>
> diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
> index a26f8ae1034dd..3f6db5b18c266 100644
> --- a/drivers/iommu/iommufd/io_pagetable.c
> +++ b/drivers/iommu/iommufd/io_pagetable.c
> @@ -465,7 +465,7 @@ iommu_read_and_clear_dirty(struct iommu_domain *domain,
> return -EOPNOTSUPP;
>
> iter = iova_bitmap_alloc(bitmap->iova, bitmap->length,
> - bitmap->page_size, bitmap->data);
> + bitmap->page_size, u64_to_user_ptr(bitmap->data));
> if (IS_ERR(iter))
> return -ENOMEM;
>
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index 50f98f01fe100..7fbf2d8a3c9b8 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -538,7 +538,7 @@ struct iommu_hwpt_get_dirty_bitmap {
> __aligned_u64 iova;
> __aligned_u64 length;
> __aligned_u64 page_size;
> - __aligned_u64 *data;
> + __aligned_u64 data;
> };
This was meant to be like this from the beginning but it appears I've made the
the mistake since the first version. Thanks for both; I will fix it and respin
it for v6.
