Re: [PATCH 09/22] arm64: dts: qcom: sm8550: Reserve low IOVA range for Iris

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Kernel panic via NULL pointer dereference in `of_iommu_get_resv_regions()` if DMA translation fails.
- [High] Incorrect iteration over `iommu-addresses` when a phandle does not match `dev->of_node`, causing parsing misalignment and out-of-bounds reads.
- [Medium] Pre-existing memory leak of `device_node` in `of_iommu_get_resv_regions()`.
--

--- Patch [9]: [PATCH 09/22] arm64: dts: qcom: sm8550: Reserve low IOVA range for Iris ---
commit b23293d9b1e601df9a40295b28db44388efa63a4
Author: Vikash Garodia <[email protected]>

arm64: dts: qcom: sm8550: Reserve low IOVA range for Iris

This commit adds a reserved-memory IOVA reservation node for the Iris VPU to
keep IOMMU allocations above 0x25800000. It prevents SMMU page faults and
device reboots when DMA accesses that range. The new region is referenced
from the Iris node using the iommu-addresses property.

> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> index 0e701e709235c..1eb725a584b26 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi

[ ... ]

> @@ -1088,6 +1088,17 @@ hyp_ext_reserved_mem: hyp-ext-reserved-region@ff700000 {
>  			reg = <0 0xff700000 0 0x100000>;
>  			no-map;
>  		};
> +
> +		/*
> +		 * The Iris VPU reserves IOVA below 0x25800000 (600MB),
> +		 * primarily for non-pixel buffers using different Stream IDs.
> +		 * DMA into that range triggers unhandled SMMU faults and
> +		 * spontaneous reboots, so reserve it to keep IOMMU
> +		 * allocations above this boundary.
> +		 */
> +		iris_iova: iris-iova {
> +			iommu-addresses = <&iris 0x0 0x0 0x0 0x25800000>;
> +		};

[Severity: High]
This is a pre-existing issue, but could introducing this iommu-addresses
property expose a panic in the device tree parsing logic?

In drivers/iommu/of_iommu.c:of_iommu_get_resv_regions(), if the DMA region
translation fails, the maps pointer is overwritten with a NULL pointer and
length is left uninitialized:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
				maps = of_translate_dma_region(np, maps, &iova, &length);
				if (length == 0) {
    ...
}

Because length is uninitialized stack memory, it might not be zero. 

The loop would then iterate again, and since maps is NULL and end is a valid
memory address, the loop condition is met and it attempts to dereference the
NULL pointer:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
		while (maps < end) {
			struct device_node *np;
			u32 phandle;

			phandle = be32_to_cpup(maps++);
    ...
}


[Severity: High]
This isn't a bug introduced by this patch, but does the IOMMU parsing logic
correctly advance the maps pointer when a phandle does not match?

When iterating over the properties, if the phandle does not match the active
device node, the maps pointer only advances by one cell:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
			np = of_find_node_by_phandle(phandle);

			if (np == dev->of_node) {
    ...
}

Without logic to advance maps past the address and size cells of the skipped
entry, subsequent iterations will mistakenly read the address or size data as
the next phandle, corrupting the array parsing and potentially causing an
out-of-bounds read.


[Severity: Medium]
This is a pre-existing issue, but is there a reference count leak for the
device node during this parsing loop?

Inside the same property parsing loop, a reference to the device node is
obtained:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
			np = of_find_node_by_phandle(phandle);
    ...
}

However, the loop iteration concludes without calling of_node_put() to drop
the reference:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
			}
		}
    ...
}

Would it be helpful to add an of_node_put(np) call after the entry is parsed
to prevent leaking the reference on each valid phandle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9