Re: [PATCH v2 1/2] iommu: Reserve PCI host bridge MMIO windows in group reserved regions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: 32-bit truncation of PCI MMIO window length causes IOVA reservation underflow
- [Medium] mm: list_splice_tail breaks sorted invariant of group reserved regions list
--

commit 29a58ada8b8ff270cc47d002ecdfe9863744ad28
Author: Guanghui Feng <guanghuifeng@linux.alibaba.com>

    iommu: Reserve PCI host bridge MMIO windows in group reserved regions

    The DMA IOVA layer reserves PCI host bridge MMIO windows to prevent
    overlaps. VFIO currently does not receive these via
    iommu_get_group_resv_regions(), allowing userspace to choose conflicting
    IOVA addresses. This introduces a helper to provide these windows as
    reserved regions.

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 58c624513cd43..c97622826d9d9 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c

[ ... ]

> @@ -508,17 +509,23 @@ static int iova_reserve_pci_windows(struct pci_dev *dev,
>  		struct iova_domain *iovad)
>  {
>  	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
> +	struct iommu_resv_region *region, *next;
>  	struct resource_entry *window;
>  	unsigned long lo, hi;
>  	phys_addr_t start = 0, end;
> +	LIST_HEAD(pci_windows);
> +	int ret;
>  
> -	resource_list_for_each_entry(window, &bridge->windows) {
> -		if (resource_type(window->res) != IORESOURCE_MEM)
> -			continue;
> +	ret = iommu_get_pci_resv_windows(dev, &pci_windows);
> +	if (ret)
> +		return ret;
>  
> -		lo = iova_pfn(iovad, window->res->start - window->offset);
> -		hi = iova_pfn(iovad, window->res->end - window->offset);
> +	list_for_each_entry_safe(region, next, &pci_windows, list) {
> +		lo = iova_pfn(iovad, region->start);
> +		hi = iova_pfn(iovad, region->start + region->length - 1);

[Severity: High]
Does this calculation result in an incorrect reservation upper bound on 32-bit
architectures with 64-bit physical addresses?

Since region->length is populated in iommu_get_pci_resv_windows() where a
64-bit size is assigned to the size_t field tmp.length, it is implicitly
truncated to 32 bits on systems like ARM LPAE or x86 PAE.

If the PCI host bridge MMIO window is >= 4GB, the truncated length causes
reserve_iova() to reserve a drastically undersized region, which might fail
to prevent IOVA allocations from overlapping with PCI MMIO ranges.

>  		reserve_iova(iovad, lo, hi);
> +		list_del(&region->list);
> +		kfree(region);
>  	}

[ ... ]

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cd1bca7ede9af..b77e9818e4a40 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c

[ ... ]

> @@ -945,6 +945,84 @@ iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_PCI
> +/**
> + * iommu_get_pci_resv_windows - collect PCI host bridge MMIO windows as
> + *                               reserved regions

[ ... ]

> +int iommu_get_pci_resv_windows(struct pci_dev *dev, struct list_head *head)
> +{
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
> +	struct iommu_resv_region *region, *next;
> +	struct resource_entry *window;
> +	LIST_HEAD(resv_windows);
> +	int ret;
> +
> +	resource_list_for_each_entry(window, &bridge->windows) {
> +		struct iommu_resv_region tmp = {
> +			.type = IOMMU_RESV_RESERVED,
> +		};
> +
> +		if (resource_type(window->res) != IORESOURCE_MEM)
> +			continue;
> +
> +		tmp.start = window->res->start - window->offset;
> +		tmp.length = window->res->end - window->res->start + 1;

[Severity: High]
Does this assignment implicitly truncate the window length on 32-bit systems?

Since window->res->end and window->res->start are 64-bit physical addresses
on architectures like ARM LPAE or x86 PAE, evaluating this difference yields
a 64-bit number. The tmp.length field is of type size_t, which is 32-bit on
these architectures.

Could this silent truncation prevent the reservation of the full bridge
window and potentially lead to PCIe peer-to-peer routing corruption?

> +
> +		ret = iommu_insert_resv_region(&tmp, &resv_windows);
> +		if (ret)
> +			goto err_free;
> +	}
> +
> +	list_splice_tail(&resv_windows, head);

[Severity: Medium]
Does list_splice_tail() break the sorted invariant of the group reserved
regions list?

When iommu_resv_pci_windows() is called by iommu_get_group_resv_regions(), the
head list may already contain device-level reserved regions (like MSI or RMRR).

Since the list is expected to be strictly sorted by start address and merged
for overlaps via iommu_insert_resv_region(), directly appending the PCI
windows to the end of head might leave the list unsorted if PCI MMIO
addresses are lower than existing regions.

Can this cause subsequent insertions to fail merging overlaps properly or
cause consumers like VFIO to encounter unsorted or overlapping regions?

> +	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921103922.1113752-1-guanghuifeng@linux.alibaba.com?part=1