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 <[email protected]> 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(®ion->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(®ion->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/[email protected]?part=2
