Re: [PATCH v6 09/11] arm64: dts: ti: k3-am62a-ti-ipc-firmware: Split r5f memory region

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing `bootph-pre-ram` on split memory regions causes dangling phandles in SPL, loss of reserved memory protection, and SPL DT parsing failures.
- [High] Splitting the contiguous memory region imposes strict address translation boundaries that break backward compatibility with legacy firmware images and older bootloaders.

Pre-existing issues:
- [High] The function `k3_get_loaded_rsc_table` hardcodes the resource table size to 256 bytes without checking the actual size of the corresponding memory region (`rmem[0]`), leading to a potential out-of-bounds read and subsequent out-of-bounds write.
--

commit b37808623c173089a1488283117154e8c2da6e07
Author: Markus Schneider-Pargmann (TI) <msp@baylibre.com>
Subject: arm64: dts: ti: k3-am62a-ti-ipc-firmware: Split r5f memory region

This commit splits the existing firmware memory region into five separate,
more specific regions. It updates the memory-region array and names to
reflect these newly separated allocations.

> diff --git a/arch/arm64/boot/dts/ti/k3-am62a-ti-ipc-firmware.dtsi b/arch/arm64/boot/dts/ti/k3-am62a-ti-ipc-firmware.dtsi
> index 682b1c9f3071d..fe10d3e75ceee 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62a-ti-ipc-firmware.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62a-ti-ipc-firmware.dtsi
> @@ -36,12 +36,36 @@ wkup_r5fss0_core0_dma_memory_region: memory@9c800000 {
>  		no-map;
>  	};
>  
> -	wkup_r5fss0_core0_memory_region: memory@9c900000 {
> +	wkup_r5fss0_core0_ipc_region: memory@9c900000 {
>  		compatible = "shared-dma-pool";
> -		reg = <0x00 0x9c900000 0x00 0x01d00000>;
> +		reg = <0x00 0x9c900000 0x00 0x100000>;
> +		no-map;
> +	};

[Severity: High]
Since this newly created ipc region (and several others below) lacks the
bootph-pre-ram property that the original firmware region had, will SPL
fdtgrep strip these nodes from the SPL device tree?

If they are stripped, the parent node wkup_r5fss0_core0 will retain phandles
to missing nodes. Could this cause dangling phandle parsing failures in
U-Boot's remoteproc driver?

> +
> +	wkup_r5fss0_core0_lpm_fs_stub_region: memory@9ca00000 {
> +		compatible = "shared-dma-pool";
> +		reg = <0x00 0x9ca00000 0x00 0x8000>;
> +		no-map;
> +	};
> +
> +	wkup_r5fss0_core0_lpm_metadata_region: memory@9ca08000 {
> +		compatible = "shared-dma-pool";
> +		reg = <0x00 0x9ca08000 0x00 0x1000>;
>  		no-map;
>  		bootph-pre-ram;
>  	};
> +
> +	wkup_r5fss0_core0_lpm_rest_region: memory@9ca09000 {
> +		compatible = "shared-dma-pool";
> +		reg = <0x00 0x9ca09000 0x00 0x97000>;
> +		no-map;
> +	};
> +
> +	wkup_r5fss0_core0_dm_region: memory@9caa0000 {
> +		compatible = "shared-dma-pool";
> +		reg = <0x00 0x9caa0000 0x00 0x1b60000>;
> +		no-map;
> +	};

[Severity: High]
Without bootph-pre-ram on wkup_r5fss0_core0_dm_region, SPL will lose the
no-map protection for this 27.3MB area. Does this mean the SPL lmb allocator
might treat this space as free DDR and allocate over it, potentially
corrupting active Device Manager firmware and causing a system hang?

>  };

[ ... ]

> @@ -78,8 +102,14 @@ &wkup_r5fss0 {
>  &wkup_r5fss0_core0 {
>  	mboxes = <&mailbox0_cluster0>, <&mbox_r5_0>;
>  	memory-region = <&wkup_r5fss0_core0_dma_memory_region>,
> -			<&wkup_r5fss0_core0_memory_region>;
> -	memory-region-names = "dma", "firmware";
> +			<&wkup_r5fss0_core0_ipc_region>,
> +			<&wkup_r5fss0_core0_lpm_fs_stub_region>,
> +			<&wkup_r5fss0_core0_lpm_metadata_region>,
> +			<&wkup_r5fss0_core0_lpm_rest_region>,
> +			<&wkup_r5fss0_core0_dm_region>;
> +	memory-region-names = "dma", "ipc", "lpm-stub",
> +			      "lpm-metadata", "lpm-context",
> +			      "dm-firmware";

[Severity: High]
Are there backwards compatibility risks with splitting the contiguous 29MB
firmware region into these individual smaller DMA pools?

If legacy bootloaders or legacy monolithic firmware images search for the
firmware string or expect index 1 to cover the full size, they will now
only find the 1MB ipc region.

Additionally, the Linux remoteproc driver strictly validates ELF segments
against individual region boundaries. If an older firmware image contains an
ELF PT_LOAD segment larger than 1MB or one that spans across these new hard
boundaries, might the kernel driver reject the translation and abort the
load?

[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but I wanted to ask about how the remoteproc driver handles resource
tables regarding these memory regions.

In drivers/remoteproc/ti_k3_common.c:k3_get_loaded_rsc_table():

	*rsc_table_sz = 256;
	return (__force struct resource_table *)kproc->rmem[0].cpu_addr;

Since the size of the resource table is hardcoded to 256 bytes without
checking the actual size of the corresponding device tree memory region
(rmem[0]), is it possible for an out-of-bounds read and subsequent write to
occur?

Later, rproc_set_rsc_table() copies 256 bytes from this pointer, and during
shutdown rproc_reset_rsc_table_on_detach() copies 256 bytes back. If the
memory region specified in the device tree happens to be smaller than 256
bytes, could this cause memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-topic-am62a-ioddr-dt-v6-19-v6-0-16afba97fbe0@baylibre.com?part=9