Re: [PATCH 17/22] arm64: dts: qcom: sc7180: 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] Flawed iteration and error handling in of_iommu_get_resv_regions() causes parser corruption and NULL pointer dereference.
--

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

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

This commit adds a reserved-memory IOVA node for the Venus VPU covering
the first 600MB. It references this node in the venus video-codec to keep
IOMMU allocations above this boundary and prevent spontaneous reboots
caused by unhandled SMMU page faults.

> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 70fbfb31d4a4d..4ea2dfe311039 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -692,6 +692,17 @@ rmtfs_mem: memory@94600000 {
>  			qcom,client-id = <1>;
>  			qcom,vmid = <QCOM_SCM_VMID_MSS_MSA>;
>  		};
> +
> +		/*
> +		 * 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 0x0 0x0 0x25800000>;

[Severity: High]
This isn't a bug introduced by this patch, but adding this iommu-addresses
property exposes a pre-existing flaw in how drivers/iommu/of_iommu.c handles
parsing in of_iommu_get_resv_regions().

If a phandle does not match dev->of_node:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
    while (maps < end) {
        ...
        phandle = be32_to_cpup(maps++);
        np = of_find_node_by_phandle(phandle);

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

Does the code fail to advance the maps pointer past the current entry's
address and size cells, corrupting subsequent parsing?

Additionally, if of_translate_dma_region() fails, it 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;
                }
    ...
}

Could this leave length and iova uninitialized and set maps to NULL, leading
to a NULL pointer dereference on the next loop iteration?

> +		};
>  	};

[ ... ]

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