Re: [PATCH 02/22] dt-bindings: media: qcom,sm8550-iris: Allow IOVA reservation memory-region

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

Pre-existing issues:
- [High] NULL pointer dereference when `of_translate_dma_region()` fails.
- [High] Parsing desynchronization when `iommu-addresses` contains entries for different devices.
--

--- Patch [2]: [PATCH 02/22] dt-bindings: media: qcom,sm8550-iris: Allow IOVA reservation memory-region ---
commit 75d07dc0e88c8eab8911e5e4c741c2ddba6cf12f
Author: Daniel J Blueman <[email protected]>

dt-bindings: media: qcom,sm8550-iris: Allow IOVA reservation memory-region

Non-pixel stream IDs should not allocate IOVA space from the restricted region,
as this region is reserved exclusively for secure non-pixel. Iris platforms
must declare an IOMMU IOVA reservation to keep DMA away from this range. This
patch permits a second memory-region phandle for this purpose and describes
each entry.

> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> index 0400ca1bff05d..7dff7293fd5c8 100644
> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> @@ -84,7 +84,10 @@ properties:
>    dma-coherent: true
>  
>    memory-region:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Firmware-loaded codec carveout
> +      - description: IOMMU IOVA reservation region

[Severity: High]
This isn't a bug introduced by this patch, but introducing memory-region nodes
utilizing iommu-addresses exposes a pre-existing NULL pointer dereference in
the boot/probe path.

When the iommu-addresses property is parsed in of_iommu_get_resv_regions(), if
of_translate_dma_region() fails (e.g. due to a missing parent bus or malformed
address), it returns NULL, which is directly assigned to the maps pointer:

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

Will this cause a kernel panic on the next iteration?

The check maps < end would evaluate to true (since end is a valid, non-zero
pointer), and the code then executes be32_to_cpup(maps++), which dereferences
the NULL pointer. Additionally, the length variable remains uninitialized on
this failure path:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
	while (maps < end) {
		struct device_node *np;
		u32 phandle;

		phandle = be32_to_cpup(maps++);
    ...
}

[Severity: High]
This is a pre-existing issue, but utilizing iommu-addresses exposes a parsing
desynchronization when the iommu-addresses property contains entries for
different devices.

If the parser reads an entry where the phandle does not match dev->of_node, it
skips processing but fails to advance the maps pointer past the address and
size cells:

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

Does this completely desynchronize the parser?

On the next iteration, it seems the parser would read the first address cell
of the skipped entry as the next phandle, making it impossible to correctly
parse a shared memory-region node containing multiple devices.

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