Re: [PATCH 12/22] arm64: dts: qcom: agatti: 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] Multiple parsing vulnerabilities in `of_iommu_get_resv_regions` when processing `iommu-addresses`.
--

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

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

This commit reserves IOVA addresses below 0x25800000 for the Venus VPU.
This prevents unhandled SMMU page faults and spontaneous reboots when
other stream IDs DMA into that range.

Fixes: 7a37bf31e3a7 ("arm64: dts: qcom: qcm2290: Add Venus video node")
Cc: [email protected]

> diff --git a/arch/arm64/boot/dts/qcom/agatti.dtsi b/arch/arm64/boot/dts/qcom/agatti.dtsi
> index 590bd2432d85b..6ae66ef882363 100644
> --- a/arch/arm64/boot/dts/qcom/agatti.dtsi
> +++ b/arch/arm64/boot/dts/qcom/agatti.dtsi
[ ... ]
> @@ -381,6 +381,17 @@ rmtfs_mem: memory@89b01000 {
>  			qcom,client-id = <1>;
>  			qcom,vmid = <QCOM_SCM_VMID_MSS_MSA QCOM_SCM_VMID_NAV>;
>  		};
> +
> +		/*
> +		 * 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 using the iommu-addresses
property here exposes multiple parsing vulnerabilities in
of_iommu_get_resv_regions() in drivers/iommu/of_iommu.c.

First, if a memory-region specifies iommu-addresses with entries for
multiple devices, the loop reads the first phandle. If it doesn't match
dev->of_node, it continues without advancing the maps pointer by the
required address and size cells (na + ns):

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

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

Does this cause the next iteration to incorrectly interpret the remaining
address and size cells as a phandle?

Second, if of_translate_dma_region() fails because there is no DMA parent,
it returns null. The stack variable length remains uninitialized. If length
is not zero, the loop proceeds.

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

Since maps is null, will this trigger a fatal null pointer dereference on
the next maps++ ?

Finally, there appears to be no bounds check before of_translate_dma_region()
reads na + ns cells from the maps pointer.

Should these parser issues in of_iommu_get_resv_regions() be fixed before
or alongside introducing this iommu-addresses usage?

[ ... ]

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