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

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

Pre-existing issues:
- [High] If `of_translate_dma_region()` fails during IOVA reservation parsing, it returns NULL, which causes a subsequent NULL pointer dereference on the next loop iteration.
- [Medium] The device node reference obtained via `of_find_node_by_phandle()` is never released, leading to a memory leak.
--

--- Patch [7]: [PATCH 07/22] arm64: dts: qcom: sc8280xp: Reserve low IOVA range for Iris ---
commit 5bb5ca70fa7d2aa5a3d66cdd7f006042161ce0dc
Author: Vikash Garodia <[email protected]>

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

This patch reserves an IOVA range for the Iris VPU to prevent unhandled
SMMU page faults when DMA accesses that range from other stream IDs. It
achieves this by adding a reserved-memory IOVA reservation node and
referencing it from the Iris node.

> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index fbcfd3883bbb4..1c624683d0033 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -669,6 +669,17 @@ reserved-region@aeb00000 {
>  			reg = <0 0xaeb00000 0 0x16600000>;
>  			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: Medium]
This is a pre-existing issue, but does of_iommu_get_resv_regions() leak the
device node reference when processing the iommu-addresses property?

When parsing these nodes, the reference is acquired via
of_find_node_by_phandle():

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

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

But it looks like of_node_put(np) is never called within the loop or the
function, which would result in a leak on every element in the property.

[Severity: High]
This is a pre-existing issue, but is there a risk of a NULL pointer
dereference here if of_translate_dma_region() fails?

If the translation fails (for example, due to an invalid DMA parent),
of_translate_dma_region() returns NULL:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
				maps = of_translate_dma_region(np, maps, &iova, &length);
				if (length == 0) {
					dev_warn(dev, "Cannot reserve IOVA region of 0 size\n");
					continue;
				}
    ...
}

Since maps is updated to NULL, the loop condition (maps < end) still
evaluates to true. On the next iteration, be32_to_cpup(maps++) would
unconditionally 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++);
    ...
}

> +		};
>  	};

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