Re: [PATCH v6 2/2] of: Add support for "iommu-ranges" on a device node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] iommu: of_iommu_derive_resv_regions() corrupts iovad->anchor on 32-bit LPAE
- [High] iommu: of_iommu_derive_resv_regions() PHYS_ADDR_MAX end overflows merge
--

commit 138d9a14b5fdb2f77facac6b1fa8a670c0862071
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

of: Add support for "iommu-ranges" on a device node

This commit adds support for parsing the "iommu-ranges" device tree property
on a device node to derive reserved IOVA regions outside the usable ranges,
and restricts "iommu-addresses" on reserved-memory nodes to identity mappings
backed by a "reg" property.

> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 5c266511db789..0428ceb50dec5 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -185,11 +185,118 @@ iommu_resv_region_get_type(struct device *dev,

[ ... ]

> +static void of_iommu_derive_resv_regions(struct device *dev, struct list_head *list)
> +{

[ ... ]

> +	list_for_each_entry_safe(pos, next_range, &ranges, node) {
> +		if (pos->start > next) {
> +			region = iommu_alloc_resv_region(next, pos->start - next, prot,
> +							 IOMMU_RESV_RESERVED, GFP_KERNEL);
> +			if (region)
> +				list_add_tail(&region->list, list);
> +		}
> +
> +		if (pos->end == PHYS_ADDR_MAX)
> +			goto exit;
> +
> +		if (pos->end >= next)
> +			next = pos->end + 1;
> +
> +		list_del(&pos->node);
> +		kfree(pos);
> +	}
> +
> +	region = iommu_alloc_resv_region(next, PHYS_ADDR_MAX - next + 1,
> +					 prot, IOMMU_RESV_RESERVED, GFP_KERNEL);
> +	if (region)
> +		list_add_tail(&region->list, list);

[Severity: Critical]
On 32-bit systems with 64-bit phys_addr_t and dma_addr_t (such as ARM LPAE
with CONFIG_PHYS_ADDR_T_64BIT=y), can reserving this trailing region up to
PHYS_ADDR_MAX corrupt iovad->anchor and lead to slab corruption when the
IOVA domain is freed?

In of_iommu_derive_resv_regions(), the trailing reserved region is allocated
with start = next and length = PHYS_ADDR_MAX - next + 1, giving it an
inclusive end address of 0xffffffffffffffffULL.

When a device attaches to a DMA domain, iova_reserve_iommu_regions() in
drivers/iommu/dma-iommu.c reserves each region:

    lo = iova_pfn(iovad, region->start);
    hi = iova_pfn(iovad, region->start + region->length - 1);
    reserve_iova(iovad, lo, hi);

Because iova_pfn() returns an unsigned long (32-bit on 32-bit architectures),
shifting 0xffffffffffffffffULL right by iova_shift(iovad) and truncating to
unsigned long yields hi = 0xffffffffUL, which is IOVA_ANCHOR (~0UL).

In drivers/iommu/iova.c:reserve_iova(), the upper-bound check compares
32-bit unsigned long values against (ULLONG_MAX >> iova_shift(iovad)), which
does not trigger on 32-bit LPAE. The rbtree walk then finds &iovad->anchor
(where pfn_lo = pfn_hi = IOVA_ANCHOR):

    for (node = rb_first(&iovad->rbroot); node; node = rb_next(node)) {
        if (__is_range_overlap(node, pfn_lo, pfn_hi)) {
            iova = to_iova(node);
            __adjust_overlap_range(iova, &pfn_lo, &pfn_hi);

__adjust_overlap_range() overwrites iovad->anchor.pfn_lo with lo. Later, when
put_iova_domain() tears down the domain and calls free_iova_mem(&iovad->anchor),
iova->pfn_lo != IOVA_ANCHOR is true and kmem_cache_free(iova_cache, &iovad->anchor)
is called on the anchor embedded inside struct iommu_dma_cookie.

[Severity: High]
Does setting the trailing reserved region's end to PHYS_ADDR_MAX also cause
unsigned integer wraparound in iommu_insert_resv_region() when merging
group reserved regions?

When iommu_get_group_resv_regions() merges device reserved regions through
iommu_insert_device_resv_regions() -> iommu_insert_resv_region() in
drivers/iommu/iommu.c:

    top_end = top->start + top->length - 1;

    if (iter->start > top_end + 1) {
        list_move_tail(&iter->list, &stack);
    } else {
        top->length = max(top_end, iter_end) - top->start + 1;

First, when top is a reserved region ending at top_end == PHYS_ADDR_MAX,
top_end + 1 wraps around to 0 in phys_addr_t arithmetic. Any subsequent
overlapping IOMMU_RESV_RESERVED region with iter->start > 0 satisfies
iter->start > 0 and fails to merge.

Second, if a reserved region starting at top->start == 0 is merged with an
overlapping or adjacent region ending at iter_end == PHYS_ADDR_MAX (for
instance, when two devices in the same IOMMU group have disjoint usable
iommu-ranges), max(top_end, iter_end) - top->start + 1 evaluates
PHYS_ADDR_MAX - 0 + 1, wrapping top->length around to 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-add_iommu_ranges-v6-0-361f3921a133@oss.qualcomm.com?part=2