Re: [PATCH 18/22] arm64: dts: qcom: sdm630: Reserve low IOVA range for Venus

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

Pre-existing issues:
- [High] Severe parsing flaws in `of_iommu_get_resv_regions()` lead to parser desynchronization, out-of-bounds reads, and a NULL pointer dereference when processing the `iommu-addresses` device tree property.
--

--- Patch [18]: [PATCH 18/22] arm64: dts: qcom: sdm630: Reserve low IOVA range for Venus ---
commit c911f79533bbf9fd3f531d014c6333ee5871bf3d
Author: Vikash Garodia <[email protected]>

arm64: dts: qcom: sdm630: Reserve low IOVA range for Venus

This commit reserves IOVA addresses below 0x25800000 for the Venus VPU to
prevent unhandled SMMU page faults. It adds a reserved-memory IOVA
reservation node and references it from the venus node.

> diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
> index a41ba0da380bb..a80ae33f2c45c 100644
> --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
[ ... ]
> @@ -518,6 +518,17 @@ mdata_mem: mpss-metadata {
>  			size = <0x0 0x4000>;
>  			no-map;
>  		};
> +
> +		/*
> +		 * The Venus 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.
> +		 */
> +		venus_iova: venus-iova {
> +			iommu-addresses = <&venus 0x0 0x25800000>;
> +		};
[Severity: High]
This is a pre-existing issue, but could this iommu-addresses property
trigger parsing flaws in of_iommu_get_resv_regions()?

Looking at 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);

		if (np == dev->of_node) {

If np != dev->of_node, the loop fails to advance maps by the number
of address and size cells for the current phandle. This causes a parser
desynchronization on the next iteration.

Additionally, a few lines later:

		maps = of_translate_dma_region(np, maps, &iova, &length);
		if (length == 0) {

The unchecked NULL return from of_translate_dma_region() is assigned
directly to maps. The uninitialized length stack variable is then checked
instead of verifying the pointer.

If of_translate_dma_region() fails and returns NULL, it can lead to a
subsequent NULL pointer dereference in the next loop iteration.

>  	};
>  
>  	smem: smem {

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