Re: [PATCH 1/4] iommufd/iova_bitmap: Clip recorded ranges to the bitmap

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Sep 25, 2026 at 06:11:37PM +0200, Andrea Parri wrote:
> IOMMU_HWPT_GET_DIRTY_BITMAP can set bits in the caller's memory past the
> end of the bitmap.  When the queried range ends inside a dirty huge
> IOPTE, the generic page-table dirty code records the whole entry, and
> iova_bitmap_set() then sets every bit up to the end of the pinned page.
> 
> iova_bitmap_mapped_range() checks a range only against the pages
> currently pinned, and iova_bitmap_mapped_length() caps only the pinning
> window, so neither limits the write.  A range that starts inside the
> bitmap but ends past it passes iova_bitmap_advance_to() on its start,
> and the write loop runs to the end of the last pinned page.
> 
> Clip the range to [bitmap->iova, bitmap->iova + bitmap->length - 1]
> before computing cur_bit and last_bit.  A range that starts before the
> bitmap now has its in-range part recorded; the unsigned underflow in
> iova_bitmap_advance_to() previously rejected it whole.
> 
> The clip runs before any pinning or indexing, so for a validated query
> range cur_bit and last_bit can no longer address outside the bitmap.  It
> only narrows the requested range; the bitmap bounds are fixed at
> allocation.
> 
> Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
> ---
>  drivers/iommu/iommufd/iova_bitmap.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c
> index dac3e657d498d..44826ae330c4b 100644
> --- a/drivers/iommu/iommufd/iova_bitmap.c
> +++ b/drivers/iommu/iommufd/iova_bitmap.c
> @@ -388,6 +388,30 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque,
>  }
>  EXPORT_SYMBOL_NS_GPL(iova_bitmap_for_each, "IOMMUFD");
>  
> +/*
> + * Trim [@iova..@iova+@length-1] to the IOVA range covered by @bitmap.
> + * Dirty trackers may report more than was asked for, for instance a whole
> + * huge IOPTE of which only a part was queried.
> + */
> +static bool iova_bitmap_clip(struct iova_bitmap *bitmap, unsigned long *iova,
> +			     size_t *length)
> +{
> +	unsigned long first = max(*iova, bitmap->iova);
> +	unsigned long last = bitmap->iova + bitmap->length - 1;
> +	unsigned long end;
> +
> +	if (!*length)
> +		return false;
> +	if (!check_add_overflow(*iova, *length - 1, &end))
> +		last = min(last, end);
> +	if (first > last)
> +		return false;
> +
> +	*iova = first;
> +	*length = last - first + 1;
> +	return true;
> +}
> +
>  /**
>   * iova_bitmap_set() - Records an IOVA range in bitmap
>   * @bitmap: IOVA bitmap
> @@ -404,6 +428,9 @@ void iova_bitmap_set(struct iova_bitmap *bitmap,
>  	struct iova_bitmap_map *mapped = &bitmap->mapped;
>  	unsigned long cur_bit, last_bit, last_page_idx;
>  
> +	if (!iova_bitmap_clip(bitmap, &iova, &length))
> +		return;

There have been several patches about the dirty stuff all together and
comprehensively this is not quite the right solution.

If the caller is providing a dirty range that exceeds the bitmap then
this is coming from something like a large PTE being dirty.

If we silently truncate that to the range of the bitmap, like this
does, then we also are silently lossing dirty marks. The userspace
will call again with an iova range stepped ahead and instead of seeing
the early blocks being dirty they will be marked as clean.

There are several bugs along these lines here as generic_pt is also
incorrectly clamping the ranges of the PTE.

So.. The right answer is for this to propogate an error when it
fails. There is another patch that provides the mechanism for this
already.

I will change this to just be:

@@ -409,6 +409,18 @@ void iova_bitmap_set(struct iova_bitmap *bitmap,
        struct iova_bitmap_map *mapped = &bitmap->mapped;
        unsigned long cur_bit, last_bit, last_page_idx;
 
+       /*
+        * If the range cannot be recorded then the system call returns failure.
+        * This is to prevent loosing dirties if userspace is expecting to step
+        * the bitmap through the IOVA space.
+        */
+       if (!length || length > bitmap->length || iova < bitmap->iova ||
+           iova - bitmap->iova > bitmap->length - length) {
+               if (!bitmap->err)
+                       bitmap->err = -EPROTO;
+               return;
+       }
+

Jason