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', so it's probably
best to add a range check plus type cast, rather than an
expensive div_u64() here.
> +/**
> + * 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.
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;
};
#define IOMMU_HWPT_GET_DIRTY_BITMAP _IO(IOMMUFD_TYPE, \
IOMMUFD_CMD_HWPT_GET_DIRTY_BITMAP)
Arnd
