Re: [PATCH 05/22] arm64: dts: qcom: lemans: 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] of_iommu_get_resv_regions() fails to advance the 'maps' pointer when a parsed phandle does not match dev->of_node, leading to corrupted DT property parsing and out-of-bounds reads.
- [High] of_iommu_get_resv_regions() will dereference a NULL pointer and use uninitialized stack variables if of_translate_dma_region() fails.
- [Medium] Device node reference count leak in of_iommu_get_resv_regions().
--

--- Patch [5]: [PATCH 05/22] arm64: dts: qcom: lemans: Reserve low IOVA range for Iris ---
commit 4e0d92ff7cab972552bdfb654d8534c1ad241835
Author: Vikash Garodia <[email protected]>

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

Iris VPU reserves IOVA addresses below 0x25800000 (600MB), primarily for
secure non-pixel buffers. DMA into that range, from any other stream id
would triggers unhandled SMMU page faults that cause spontaneous device
reboots.

Fixes: 7bc95052c64f ("arm64: dts: qcom: sa8775p: add support for video node")
Cc: [email protected]

> diff --git a/arch/arm64/boot/dts/qcom/lemans.dtsi b/arch/arm64/boot/dts/qcom/lemans.dtsi
> index 695eae1b72569..da0326fe24485 100644
> --- a/arch/arm64/boot/dts/qcom/lemans.dtsi
> +++ b/arch/arm64/boot/dts/qcom/lemans.dtsi
> @@ -862,6 +862,17 @@ cpucp_fw_mem: cpucp-fw@db200000 {
>  			reg = <0x0 0xdb200000 0x0 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 isn't a bug introduced by this patch, but adding this property might
trigger a pre-existing issue in
drivers/iommu/of_iommu.c:of_iommu_get_resv_regions().

If the iommu-addresses property contains multiple entries and the first
entry's phandle does not match dev->of_node, the code skips the if block
but fails to advance the maps pointer:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
	...
	if (np == dev->of_node) {
		// ... parses and advances maps ...
	}
	// MISSING ELSE BRANCH to advance maps pointer
	...
}

Will this cause the next iteration to incorrectly interpret the first address
cell of the unmatched entry as a phandle, reducing the parsing to a corrupted
word-by-word scan?

[Severity: Medium]
This is a pre-existing issue, but does of_iommu_get_resv_regions() leak the
device node reference count when parsing these iommu-addresses?

Every iteration of the loop fetches the node, but there is no corresponding
of_node_put(np) to release it:

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

[Severity: High]
This is also a pre-existing issue, but what happens if
of_translate_dma_region() fails when parsing this property?

The maps pointer is reassigned to the return value without checking for NULL,
and the function relies on the uninitialized iova and length variables on
failure:

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

If of_translate_dma_region() returns NULL, the maps pointer is assigned NULL,
and the loop condition maps < end evaluates to true. Will the next iteration
then dereference the NULL maps pointer?

> +		};
>  	};

[ ... ]

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