Re: [PATCH v2] rockchip: px30: Fix TPL_STACK overflow into BootROM reserved region

Hi Jonas,

On 8/10/26 11:14 PM, Jonas Karlman wrote:
> The PX30/RK3326 has 16 KiB SRAM at [0xff0e0000, 0xff0e4000), with the
> initial 4 KiB at [0xff0e0000, 0xff0e1000) reserved for BootROM at boot.
> 
> SRAM addressing only seems to use 14 bits, meaning that reading from
> 0xff0e4000+ wraps around and instead reads back data at 0xff0e0000+.
> 
> Using a TPL_STACK at 0xff0e4ff0 (16-bytes aligned) means TPL use BootROM
> reserved region for its global data, malloc area and runtime stack.
> 

This should be its own commit.

> TPL on PX30/RK3326 does not have any use for the malloc area, the
> generated GD_SIZE is typically around 304 bytes and BSS around 72 bytes.
> 

As far as I understood, this is space/size optimization and not a bug 
fix, so should therefore be its own commit.

> Current memory layout possibly looks something like:
> 
>    [0xff0e0000, 0xff0e4000)          16 KiB   SRAM
>      [0xff0e0000, 0xff0e1000)         4 KiB   BROM area
>        [0xff0e09f0, 0xff0e0ff0)    1536 bytes  malloc area (unused)
>        [0xff0e08c0, 0xff0e09f0)     304 bytes  global data
>        [0xff0e08c0]                            stack pointer
>      [0xff0e1000, 0xff0e4000)        12 KiB   TPL area
>        [0xff0e1000, 0xff0e3800)      10 KiB    code
>        [0xff0e3800, 0xff0e3848)      72 bytes  BSS
> 
> Update TPL_STACK to use the top of SRAM address and reduce MALLOC_F_LEN
> to 0x0 to avoid overflowing global data and stack into BootROM reserved
> region.
> 

Aren't we then missing a build-time check to make sure this is not possible?

> After those changes the memory layout looks something like:
> 
>    [0xff0e0000, 0xff0e4000)          16 KiB   SRAM
>      [0xff0e0000, 0xff0e1000)         4 KiB   BROM area
>      [0xff0e1000, 0xff0e4000)        12 KiB   TPL area
>        [0xff0e1000, 0xff0e3800)      10 KiB    code
>        [0xff0e3800, 0xff0e3848)      72 bytes  BSS
>        [0xff0e3ed0, 0xff0e4000)     304 bytes  global data
>        [0xff0e3ed0]                            stack pointer
>          [0xff0e3848, 0xff0e3ed0)  1672 bytes  stack area
> 
> With these changes the TPL_MAX_SIZE can safely be extended to 11 KiB,
> leaving enough for the stack to grow down around 648 bytes before it
> starts to overlap into TPL or BSS.
> 
> And with all changes the memory layout looks something like:
> 
>    [0xff0e0000, 0xff0e4000)          16 KiB   SRAM
>      [0xff0e0000, 0xff0e1000)         4 KiB   BROM area
>      [0xff0e1000, 0xff0e4000)        12 KiB   TPL area
>        [0xff0e1000, 0xff0e3c00)      11 KiB    code
>        [0xff0e3c00, 0xff0e3c48)      72 bytes  BSS
>        [0xff0e3ed0, 0xff0e4000)     304 bytes  global data
>        [0xff0e3ed0]                            stack pointer
>          [0xff0e3c48, 0xff0e3ed0)   648 bytes  stack area
> 
> Signed-off-by: Jonas Karlman <[email protected]>
> Reviewed-by: Simon Glass <[email protected]>
> ---
> The overflow/wrapping was runtime validated on a RK3326 ODROID-GO Super
> by printing out content of [0xff0e0000, 0xff0e5000] and comparing the
> initial 4 KiB with the 16-20 KiB and the data matched 1:1.
> 

How did you test that? md.l on either address results in a Synchronous 
Abort on my PX30 without your patch. We could also just write a random 
pattern only at one address, and if it appears at the other address as 
well, then you've discovered an address wrap indeed.

> Changes in v2:
> - Mention BSS in commit message
> - Update spelling in commit message
> - Collect r-b tag
> 
> With the patch "global_data: Only include driver model fields when DM is
> enabled" [1], the stack area can grow down an additional 48 bytes before
> it starts to overlap into TPL or BSS.
> 
> [1] https://patch.msgid.link/[email protected]/
> ---
>   arch/arm/mach-rockchip/px30/Kconfig | 4 ++--
>   common/spl/Kconfig.tpl              | 2 +-
>   tools/rkcommon.c                    | 2 +-
>   3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/px30/Kconfig b/arch/arm/mach-rockchip/px30/Kconfig
> index adba1b49a52b..cc0632cf7b2d 100644
> --- a/arch/arm/mach-rockchip/px30/Kconfig
> +++ b/arch/arm/mach-rockchip/px30/Kconfig
> @@ -81,10 +81,10 @@ config TPL_TEXT_BASE
>   	default 0xff0e1000
>   
>   config TPL_STACK
> -	default 0xff0e4fff
> +	default 0xff0e4000
>   
>   config TPL_SYS_MALLOC_F_LEN
> -	default 0x600
> +	default 0x0

Disable malloc in TPL on PX30 instead, that's actually what you want to 
be doing and not "reserve 0 bytes for malloc". That's gated by 
TPL_SYS_MALLOC_F.

>   
>   config DEBUG_UART_CHANNEL
>   	int "Mux channel to use for debug UART2/UART3"
> diff --git a/common/spl/Kconfig.tpl b/common/spl/Kconfig.tpl
> index a535b61ecd35..cdb9cb188c7c 100644
> --- a/common/spl/Kconfig.tpl
> +++ b/common/spl/Kconfig.tpl
> @@ -129,7 +129,7 @@ config TPL_MAX_SIZE
>   	default 0x2e000 if ROCKCHIP_RK3399
>   	default 0x8000 if ROCKCHIP_RK3288 || ROCKCHIP_RV1126
>   	default 0x7000 if ROCKCHIP_RK322X || ROCKCHIP_RK3328 || ROCKCHIP_RK3368
> -	default 0x2800 if ROCKCHIP_PX30
> +	default 0x2c00 if ROCKCHIP_PX30
>   	default 0x0
>   	help
>   	  The maximum size (in bytes) of the TPL stage.
> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
> index b39777fc0607..872fc21c039d 100644
> --- a/tools/rkcommon.c
> +++ b/tools/rkcommon.c
> @@ -137,7 +137,7 @@ struct spl_info {
>   };
>   
>   static struct spl_info spl_infos[] = {
> -	{ "px30", "RK33", 0x2800, false, RK_HEADER_V1 },
> +	{ "px30", "RK33", 0x2c00, false, RK_HEADER_V1 },

Downstream actually has 0x3000 there with the justification that SRAM is 
16KiB and BootROM has 4KiB reserved for the stack, c.f. 3bbe0d2f9129 
("tools: rkcommon: add rk3326 and correct spl size for px30"), why are 
we still 0x400 below?

Cheers,
Quentin