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

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

Pre-existing issues:
- [High] Pre-existing parsing bug in `of_iommu_get_resv_regions()` causing misalignment and incorrect parsing when encountering non-matching shared reservations.
- [High] Missing NULL check for the return value of `of_translate_dma_region()` causes a kernel panic if address translation fails.
--

--- Patch [10]: [PATCH 10/22] arm64: dts: qcom: sm8650: Reserve low IOVA range for Iris ---
commit 0235003bbbcd1eb35cb1bdeb1f805b1bd98ddfd1
Author: Vikash Garodia <[email protected]>

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

Iris VPU reserves IOVA addresses below 0x25800000, primarily for secure
non-pixel buffers. This commit adds a reserved-memory IOVA reservation
node covering [0, 0x25800000] and references it from the Iris node to keep
DMA allocations above that boundary.

> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
[ ... ]
> @@ -5266,7 +5277,7 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>  			interconnect-names = "cpu-cfg",
>  					     "video-mem";
>  
> -			memory-region = <&video_mem>;
> +			memory-region = <&video_mem>, <&iris_iova>;

[Severity: High]
This isn't a bug introduced by this patch, but does adding multiple entries
to a memory-region expose a parsing bug in of_iommu_get_resv_regions()?

When processing a node with multiple entries, if an earlier phandle in the
list does not match dev->of_node, the loop in drivers/iommu/of_iommu.c
fails to advance the maps pointer over the address and size cells:

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) {
        ...
        maps = of_translate_dma_region(np, maps, &iova, &length);
        ...
    }
    /* Missing advancement of maps if np != dev->of_node */
}

Will the next iteration erroneously read the unparsed address or size cells
as the next phandle, corrupting the device tree parsing?

[Severity: High]
This is another pre-existing issue in the same parsing area, but could
address translation failures lead to a kernel panic here?

If of_translate_dma_region() fails (for example, if the device lacks a DMA
parent), it returns NULL. The return value is assigned directly to maps
without checking:

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

On the next loop iteration, the condition maps < end will be evaluated
where NULL is less than the valid kernel pointer end, and
be32_to_cpup(maps++) will dereference the NULL pointer.

Could this trigger a crash on boot if an untranslatable DMA region is
encountered?

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