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

Hi Jonas,

On 8/24/26 5:59 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 8/24/2026 3:45 PM, Quentin Schulz wrote:
>> 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.
> 
> The main purpose of this patch is to fix the overflow of TPL into lower
> 4 KiB of SRAM, i.e. the area used as BootROMs stack and state. Returning
> to BootROM to load next image (SPL) could possible be affected depending
> on TPLs use of malloc and/or its stack usage.
> 

Yeah but that's already the case no? It's not because there is one issue 
that there's necessarily one cause to the issue, or many ways to trigger 
the issue.

> All changes have been kept in a single patch to make changes safe for
> bisect. If we just change TPL_STACK and keep malloc size, or just change

Bisect isn't necessarily useful if we start with a broken state. It 
matters if we somehow make it worse, which we shouldn't.

> malloc size TPL may start to accidentally overwrite areas in the BootROM
> reserved area that affects BootROMs ability to load next image or the
> TPL stack may start to overwrite end of TPL image.
> 
> E.g. if we only change TPL_STACK we end up with 136 byte free for
> stack usage before stack can start to overwrite TPL BSS/code:
> 
>      [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
>          [0xff0e38d0]                            stack pointer
>            [0xff0e3848, 0xff0e38d0]   136 bytes  stack area
>          [0xff0e38d0, 0xff0e3ed0)    1536 bytes  malloc area (unused)
>          [0xff0e3ed0, 0xff0e4000)     304 bytes  global data
> 
> And if we change malloc size the TPL stack likely move closer to
> BootROMs own stack. Current combo of TPL_STACK, GD_SIZE and MALLOC_F_LEN
> seem to not affect BootROM but that could change if any one of them
> changes independently.
> 
>>
>>> 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.
> 
> The only part that possible safely can be moved to its own commit is the
> increase to 11 KiB for code size. Mostly wanted to keep this together
> to help for future references as this patch then have all details on why
> such size change was possible and could be considered safe.
> 

Once the issues are fixed and merged, you can always point at a git 
commit hash in another patch.

>>
>>> 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?
> 
> Not sure how we could build-time check this without a guesstimate on how
> deep runtime stack is needed.
> 

Fair enough, we know the pool we've reserved for the heap (malloc) but 
we cannot reserve space for the stack so we cannot know.

>>
>>> 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.
> 
> I used something like following to dump content as early as possible
> from TPL. I used something that printed multiple columns, and compared

That was the hint I was missing. This likely means TF-A protects the 
area somehow. I removed TF-A from u-boot.itb and I could compare both 
addresses with:

=> cmp.l 0xff0e0000 0xff0e4000 0x1000
Total of 4096 word(s) were the same

Funnily enough, I can increase the area to check to 0x3000 words but 
anything after that results in a Synchronous abort.

> the output for 0x0-0x1000 and 0x4000-0x5000. Think I also tried to write
> something and could see the values being reflected in both places.
> 

=> md.l 0xff0e0000 0x4
ff0e0000: 62144471 b9d836bc 5ef2ce5b e00c1527  qD.b.6..[..^'...
=> random 0xff0e0000 0x10
=> md.l 0xff0e0000 0x4
ff0e0000: a727051d b7145d3f aac7cfa6 3557e1df  ..'.?]........W5
=> md.l 0xff0e4000 0x4
ff0e4000: a727051d b7145d3f aac7cfa6 3557e1df  ..'.?]........W5
=> cmp.l 0xff0e0000 0xff0e4000 0x1000
Total of 4096 word(s) were the same

=> random 0xff0e0000 0x4000
16384 bytes filled with random data
=> cmp.l 0xff0e0000 0xff0e4000 0x1000
Total of 4096 word(s) were the same
=> random 0xff0e4000 0x4000
16384 bytes filled with random data
=> cmp.l 0xff0e0000 0xff0e4000 0x1000
Total of 4096 word(s) were the same

So I think this proves what you've just said.

> void tpl_board_init(void)
> {
> 	for (int i = 0; i < 0x5000 / sizeof(u32); i++) {
> 		ulong addr = SRAM_BASE + i * sizeof(u32);
> 		u32 val = readl(addr);
> 		if (!val)
> 			continue;
> 		printascii("SRAM ");
> 		printhex8(addr);
> 		printascii(": ");
> 		printhex8(val);
> 		printascii("\n");
> 	}
> }
> 
>>
>>> 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.
> 
> Ideally we should disable TPL_SYS_MALLOC_F, but I thought it was too
> obtrusive to change the default value for TPL_SYS_MALLOC_F as part of
> this fix, so opted to just use 0x0 as size limit as there was no
> difference in code for the resulting TPL binary. The only benefit of
> TPL_SYS_MALLOC_F being disabled, beside being schematically more correct,> is that GD_SIZE could be around 24 bytes smaller.
> 
> I can set the TPL_SYS_MALLOC_F_LEN to 0x200 and fully drop the 10 to 11
> KiB code change in a v3 if you prefer that over this?
> 

If we don't malloc() today, does it matter how much TPL_SYS_MALLOC_F_LEN 
is? I won't take the TPL_SYS_MALLOC_F_LEN = 0 change, now or later. If 
we really can disable malloc in TPL without making our lives much harder 
in the future, I still think we should aim for separate commits for 
disabling it and then reclaiming its space on the SRAM for the TPL max size.

Is there any chance you can check we really do not use malloc() (and 
associated, like realloc, memalign, valloc, pvalloc, calloc, 
malloc_trim, ...) in TPL, I'm a bit concerned by what this will limit us 
to in the future as well. The helptext only says the driver model (I'm 
assuming DM/xPL_DM) needs this but it isn't actually enforced at the 
Kconfig-level of the DM/xPL_DM symbols.

>>
>>>    
>>>    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?
> 
> I actually tried to change to use 0x3000 value in a prior patch, but
> dropped the change due to a response from Kever [1].
> 

I understood Kever's comment as "only take what's in downstream, no 
extra changes" and 0x3000 is coming from downstream. From looking at the 
changes, the RK3368 one you suggested back then is different from what's 
downstream (0x10000 - 0x1000 instead of 0x8000 - 0x1000), same for 
RK3288 with 0x18000 - 0x1000 vs 0x8000. The rest is identical in result 
although they may be written differently (e.g. 0x2000 - 0x800 instead of 
0x1800).

> For most RK SoCs mkimage use a hard size limit and only exclude the BROM
> reserved area, and for some reason on PX30 the size limit also likely
> leave room for U-Boot TPL runtime needs, e.g. BSS/malloc/stack use.
> 
> In my opinion the mkimage tool should not try to impose any limit on
> what the packaged image does or require during runtime, it should just
> be a simple tool that packages binary images into bootable idblock
> format and only apply hard limits.

We need mkimage to validate the generated image is usable. I think 
0x2800 for the usable SRAM size for PX30 was a mistake made by Rockchip 
in their downstream and we just used that when upstreaming support for 
it. Downstream fixed it in 3bbe0d2f9129 ("tools: rkcommon: add rk3326 
and correct spl size for px30"), dated "Fri Jan 7 15:22:54 2022 +0800", 
while upstream support for PX30 (in tools/rkcommon.c) got added in 
c6e66b12e4cd ("rockchip: mkimage: add support for px30"), dated "Fri Jul 
12 11:43:34 2019 +0200", so it's just that nobody bothered fixing this 
upstream yet.

I don't know if we take into account the malloc pool size when computing 
the size of the binary (or if we should), but there's a tool for that I 
believe in tools/spl_size_limit.c that's triggered if 
CONFIG_SPL_SIZE_LIMIT != 0x0.

Cheers,
Quentin