Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] arm: dma-mapping: off-by-one size causes mixed semantic in aperture check --
commit 5535bede81dc83f33cc8029ff49a58ed02614672 Author: Mikko Perttunen <[email protected]> ARM: dma-mapping: Bypass the legacy path for core-owned DMA API domains This commit updates the ARM DMA mapping code to bypass custom IOMMU domain management when a DMA default domain is requested and owned by the core. It adds checks in the setup and teardown routines to skip the legacy path and ensures that the device's dma-ranges cover the IOMMU aperture. > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index f2d3e8ec34872..d6315baeddb9c 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -1761,13 +1761,33 @@ EXPORT_SYMBOL_GPL(arm_iommu_detach_device); > > static void arm_setup_iommu_dma_ops(struct device *dev) > { > - struct dma_iommu_mapping *mapping; > + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); > u64 dma_base = 0, size = 1ULL << 32; > > if (dev->dma_range_map) { > dma_base = dma_range_map_min(dev->dma_range_map); > size = dma_range_map_max(dev->dma_range_map) - dma_base; > } > + > + /* > + * An existing cookie means the core owns the domain. Only set the > + * per-binding DMA ops. IOVAs are allocated from the whole aperture, > + * so the device's dma-ranges must cover it. > + */ > + if (mapping) { > + const struct iommu_domain_geometry *geo = > + &mapping->domain->geometry; > + > + if (dma_base > geo->aperture_start || > + dma_base + size < geo->aperture_end) { [Severity: Medium] Does the new aperture check incorrectly mix inclusive and exclusive size semantics? When the device has a dma_range_map, the size calculation relies on dma_range_map_max(), which returns an inclusive maximum address: include/linux/dma-direct.h:dma_range_map_max() { ... for (; map->size; map++) ret = max(ret, map->dma_start + map->size - 1); return ret; } Since dma_range_map_max() is inclusive, size = max - min results in a value 1 byte too small. The new check (dma_base + size < geo->aperture_end) appears to mathematically rely on this off-by-one size to evaluate correctly against an inclusive aperture_end. If the size calculation is ever corrected, would this incorrectly accept dma-ranges that are 1 byte smaller than the aperture? Furthermore, when dma_range_map is absent, size defaults to 1ULL << 32 (exclusive). If aperture_end is exactly 0x100000000, will this check incorrectly fail? > + dev_err(dev, "dma-ranges do not cover the IOMMU > aperture\n"); > + return; > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
