Re: [RFC PATCH 08/11] arm64: dts: qcom: sm8550: Move Iris IOMMUs to child nodes

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `reservation-iris` node uses `iommu-addresses` without a `reg` property. In `of_iommu_get_resv_regions`, this is parsed as an `IOMMU_RESV_RESERVED` region. The IOMMU subsystem explicitly reserves these regions in the IOVA allocator tree (e.g., in `reserve_iova`), which prevents any DMA allocations from using this memory range. Instead of limiting the non-pixel stream to the 0-600 MB IOVA range, this patch effectively denylists the 0-600 MB range, forcing all allocations to use IOVAs above 600 MB. This inverts the intended hardware constraint and will cause the VPU crashes it aims to prevent.
- [High] The `non-pixel` and `pixel` child nodes lack `compatible` strings and do not bind to any platform driver. The driver registers them using `platform_device_register_full()`, but because no driver probes them, the driver core never calls `platform_dma_configure()` (and thus `of_dma_configure()`) for these devices. Consequently, their `dev->iommu_fwspec` and DMA ops remain unconfigured. When `dma_alloc_attrs()` is later called using these context bank devices, the allocations will bypass the IOMMU (falling back to direct physical memory allocations) and trigger SMMU translation faults. The driver must manually invoke `of_dma_configure()` on the created child devices.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but in `of_iommu_get_resv_regions`, if `of_translate_dma_region()` fails (e.g., crossing a non-translatable boundary or encountering a malformed `dma-ranges`), it returns `NULL`, which is assigned to the `maps` pointer. Because `length` is uninitialized, it can be non-zero, allowing the code to bypass the `if (length == 0)` check. The loop condition `while (maps < end)` evaluates to true (since `NULL < end`), and the next iteration immediately dereferences the `NULL` pointer at `be32_to_cpup(maps++)`, resulting in a kernel panic.
- [High] This problem wasn't introduced by this patch, but `of_iommu_get_resv_regions` lacks bounds checking when parsing `iommu-addresses`. The loop blindly advances `maps` cell by cell, without skipping the address and size data for unmatched phandles. If a data cell coincidentally matches `dev->of_node`'s phandle, or if the property is truncated, `of_translate_dma_region` delegates to `__of_translate_address`, which executes `memcpy(addr, in_addr, na * 4)` and `of_read_number(prop + na, ns)` without ensuring there are `na + ns` cells remaining before `end`. This leads to an out-of-bounds read of the device tree property.
--

--- Patch [8]: [RFC PATCH 08/11] arm64: dts: qcom: sm8550: Move Iris IOMMUs to child nodes ---
commit 84b6e774da5044311b95ef6d8c3458ace387439e
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
Subject: arm64: dts: qcom: sm8550: Move Iris IOMMUs to child nodes

This commit separates the Iris VPU IOMMUs into pixel and non-pixel child
nodes to describe their different IOVA constraints. It also attempts to
restrict the non-pixel stream to the 0-600 MB IOVA range using a memory
reservation node.

> diff --git a/arch/arm64/boot/dts/qcom/qcs8550.dtsi b/arch/arm64/boot/dts/qcom/qcs8550.dtsi
> --- a/arch/arm64/boot/dts/qcom/qcs8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs8550.dtsi

[ ... ]

> @@ -158,5 +158,9 @@ mpss_dsm_mem: mpss-dsm-region@d4d00000 {
>  			reg = <0x0 0xd4d00000 0x0 0x3300000>;
>  			no-map;
>  		};
> +
> +		iris_resv: reservation-iris {
> +			iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>;
> +		};
>  	};
>  };

[Severity: High]
Does this configuration actually reserve the 0-600 MB range for the non-pixel
stream to use, or does it unintentionally prevent its use?

Since the reservation-iris node uses iommu-addresses without a reg property,
the IOMMU subsystem parses this as an IOMMU_RESV_RESERVED region. The IOVA
allocator explicitly denylists reserved regions to protect them, preventing
any DMA allocations from using that memory range.

Will this force all non-pixel stream allocations into higher addresses
outside the 0-600 MB range, potentially causing the VPU crashes this aims
to prevent?

[Severity: High]
This isn't a bug introduced by this patch, but does the addition of the
iommu-addresses property expose an unhandled failure path in the IOMMU core?

If address translation fails for any reason (like a malformed property),
of_translate_dma_region() returns a NULL pointer. Because the length variable
is uninitialized, the code can bypass the length == 0 check:

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

The loop condition while (maps < end) remains true, and the next iteration
will immediately dereference the NULL pointer at be32_to_cpup(maps++),
causing a kernel panic. 

[Severity: High]
This is also a pre-existing issue, but does parsing the iommu-addresses
property risk an out-of-bounds read?

The loop in of_iommu_get_resv_regions() advances the maps pointer cell by
cell. If a data cell coincidentally matches a device phandle, it calls
of_translate_dma_region() which blindly delegates the copy:

drivers/of/address.c:__of_translate_address() {
	...
	memcpy(addr, in_addr, na * 4);
	...
}

If there are not enough cells remaining before the end of the property
array, could this read past the allocated bounds of the device tree
property?

> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi

[ ... ]

> @@ -3690,10 +3694,11 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>  			resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>  			reset-names = "bus";
>  
> -			iommus = <&apps_smmu 0x1940 0>,
> -				 <&apps_smmu 0x1947 0>;
>  			dma-coherent;
>  
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +
>  			/*
>  			 * IRIS firmware is signed by vendors, only
>  			 * enable on boards where the proper signed firmware
> @@ -3701,6 +3706,15 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>  			 */
>  			status = "disabled";
>  
> +			iris_non_pixel: non-pixel {
> +				iommus = <&apps_smmu 0x1940 0x0>;
> +				memory-region = <&iris_resv>;
> +			};
> +
> +			iris_pixel: pixel {
> +				iommus = <&apps_smmu 0x1947 0x0>;
> +			};
> +
>  			iris_opp_table: opp-table {
>  				compatible = "operating-points-v2";

[Severity: High]
Because these child nodes lack compatible strings, they do not bind to any
platform driver. When the driver registers them, is their DMA configuration
left uninitialized?

The driver core calls platform_dma_configure() automatically when a driver
probes. Without a driver binding, the device's iommu_fwspec and DMA ops
remain unconfigured:

drivers/media/platform/qcom/iris/iris_resources.c:iris_create_cb_dev() {
	...
	pdev = platform_device_register_full(&plat_dev_info);
	/* of_dma_configure() is not called manually here */
}

When the driver later calls dma_alloc_attrs() for these context bank
devices, will it bypass the IOMMU entirely and trigger SMMU translation
faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-vpu_iommu_iova_handling-v1-0-72bb62cb2dfd@oss.qualcomm.com?part=8