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

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

Pre-existing issues:
- [High] Missing bounds check, incorrect pointer advancement, and potential NULL pointer dereference when parsing the `iommu-addresses` property.
--

--- Patch [4]: [PATCH 04/22] arm64: dts: qcom: hamoa: Reserve low IOVA range for Iris ---
commit 0d7d545a5dddd31b4e0cc75f7a486bdd8ebe240d
Author: Daniel J Blueman <[email protected]>

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

This commit adds a reserved iova region below 0x25800000 for the Iris vpu on
hamoa platforms to prevent smmu faults. It references this reserved memory
using the iommu-addresses property to keep dma allocations above the boundary.

Link: https://github.com/qualcomm-linux/kernel-topics/issues/1157#issuecomment-4458933574

> diff --git a/arch/arm64/boot/dts/qcom/hamoa.dtsi b/arch/arm64/boot/dts/qcom/hamoa.dtsi
> index 8a96ea1bdb9c4..87f424765d513 100644
> --- a/arch/arm64/boot/dts/qcom/hamoa.dtsi
> +++ b/arch/arm64/boot/dts/qcom/hamoa.dtsi
> @@ -666,6 +666,17 @@ smem_mem: smem@ffe00000 {
>  			hwlocks = <&tcsr_mutex 3>;
>  			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 iommu-addresses
property could trigger a pre-existing issue in the device tree parsing logic.

Looking at of_iommu_get_resv_regions() in drivers/iommu/of_iommu.c, when
parsing the iommu-addresses property, if a parsed phandle does not match
dev->of_node, the loop fails to skip the parent's na + ns cells:

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) {
			// ...
			maps = of_translate_dma_region(np, maps, &iova, &length);
    ...
}

Because maps is only incremented by 1 cell (the phandle) instead of
1 + na + ns, the parser becomes misaligned. Subsequent iterations will
interpret address and size cells as phandles.

Additionally, if of_translate_dma_region() fails, it returns null. The loop
then assigns null to maps. Since end is a valid kernel pointer, the maps < end
check passes on the next iteration, and be32_to_cpup(maps++) dereferences the
null pointer.

Furthermore, in of_translate_dma_region(), there is no bounds checking against
the property's allocation size:

drivers/of/address.c:of_translate_dma_region() {
    ...
	address = of_translate_dma_address(dev, prop);
	if (address == OF_BAD_ADDR)
		return NULL;

	size = of_read_number(prop + na, ns);
    ...
}

If the parser is misaligned and a false match occurs near the end of the
buffer, this can read out-of-bounds heap memory.

Does this new device tree property risk exposing these parsing issues if
multiple entries or malformed data are present in the device tree?

> +		};
>  	};

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