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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iommu: PCI window length truncated to 32-bit on PAE systems
- [Medium] iommu: Reserved regions list sorting and merging invariants broken
- [Medium] iommu: Missed host bridge window reservations in shared IOMMU groups
--

commit 7e6d6a860340f3d138fa7f3fa2d3327a38aa9b7b
Author: Guanghui Feng <guanghuifeng@linux.alibaba.com>

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

This commit introduces iommu_get_pci_resv_windows() to collect PCI host bridge
MMIO windows as reserved regions. It updates iova_reserve_pci_windows() to use
this helper and adds iommu_resv_pci_windows() to reserve these windows at the
group level to prevent IOVA overlaps.

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 58c624513cd43..bccb530434629 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
[ ... ]
> @@ -508,18 +509,26 @@ 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);
>  
> -		lo = iova_pfn(iovad, window->res->start - window->offset);
> -		hi = iova_pfn(iovad, window->res->end - window->offset);
> -		reserve_iova(iovad, lo, hi);
> +	list_for_each_entry_safe(region, next, &pci_windows, list) {
> +		if (!ret) {
> +			lo = iova_pfn(iovad, region->start);
> +			hi = iova_pfn(iovad, region->start + region->length - 1);
> +			reserve_iova(iovad, lo, hi);

[Severity: High]
Does this length calculation risk passing inverted or truncated ranges to
reserve_iova()?

If region->length gets truncated in iommu_get_pci_resv_windows(), the
truncated length will be used here to reconstruct the upper bound, which can
result in an underflow. This could insert inverted ranges into the IOVA
rbtree, corrupting its invariants.

> +		}
> +		list_del(&region->list);
> +		kfree(region);
>  	}
> +	if (ret)
> +		return ret;
[ ... ]
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index cd1bca7ede9af..de675ed277535 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
[ ... ]
> +#ifdef CONFIG_PCI
> +/**
> + * iommu_get_pci_resv_windows - collect PCI host bridge MMIO windows as
> + *                               reserved regions
> + * @dev: PCI device whose host bridge to scan
> + * @head: list head to append iommu_resv_region entries to
> + *
> + * Walks the MMIO windows of @dev's PCI host bridge and creates an
> + * IOMMU_RESV_RESERVED region for each one.  The caller must free the
> + * returned entries with kfree() when done.
> + *
> + * Returns 0 on success, negative errno on failure.
> + */
> +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 resource_entry *window;
> +
> +	resource_list_for_each_entry(window, &bridge->windows) {
> +		struct iommu_resv_region *region;
> +		phys_addr_t start;
> +		size_t length;
> +
> +		if (resource_type(window->res) != IORESOURCE_MEM)
> +			continue;
> +
> +		start = window->res->start - window->offset;
> +		length = window->res->end - window->res->start + 1;

[Severity: High]
Can this truncate the resource size?

On 32-bit systems with PAE/LPAE enabled, resource_size_t is 64-bit while
size_t length is 32-bit. If the MMIO window is >= 4GB, the 64-bit size is
implicitly truncated when assigned here.

> +
> +		region = iommu_alloc_resv_region(start, length, 0,
> +						 IOMMU_RESV_RESERVED,
> +						 GFP_KERNEL);
> +		if (!region)
> +			return -ENOMEM;
> +
> +		list_add_tail(&region->list, head);

[Severity: Medium]
Does this break the sorting and merging invariants of the reserved regions
list?

Directly appending the newly allocated PCI window regions to the end of the
list bypasses the iommu_insert_resv_region() insertion sort and merge logic.
Consumers like VFIO that expect a normalized list will be fed unsorted,
potentially overlapping address spaces.

> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_get_pci_resv_windows);
> +
> +/*
> + * Reserve PCI host bridge MMIO windows as IOMMU_RESV_RESERVED regions.
> + * This prevents IOVA allocations from overlapping with PCI MMIO address
> + * ranges, which could cause PCIe switches to misroute DMA transactions.
> + *
> + * All PCI devices within the same IOMMU group share the same host bridge,
> + * so we only need to find the first PCI device.
> + *
> + * Caller must hold group->mutex.
> + */
> +static int iommu_resv_pci_windows(struct iommu_group *group,
> +				  struct list_head *head)
> +{
> +	struct group_device *gdev;
> +
> +	for_each_group_device(group, gdev) {
> +		if (!dev_is_pci(gdev->dev))
> +			continue;
> +		return iommu_get_pci_resv_windows(to_pci_dev(gdev->dev),
> +						  head);

[Severity: Medium]
Will this skip subsequent devices that might belong to different host bridges
within the same IOMMU group?

If an IOMMU group contains PCI devices from multiple distinct PCI host
bridges (e.g., on platforms lacking hardware isolation), returning
immediately after finding the first PCI device means the MMIO windows for all
other bridges remain unprotected.

> +	}
> +	return 0;
> +}
> +#endif /* CONFIG_PCI */

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