[PATCH] spl: spl_legacy: Fix spl_end address
From: Fabio Estevam festevam@denx.de
Currently, spl_end points to the __bss_end address, which is an external RAM address instead of the end of the SPL text section in the internal RAM.
This causes boot failures on imx6-colibri, for example:
``` Trying to boot from MMC1 SPL: Image overlaps SPL resetting ... ``` Fix this problem by assigning spl_end to the _image_binary_end, as this symbol properly represents the end of the SPL text section.
From u-boot-spl.map:
.end *(.__end) 0x00000000009121a4 _image_binary_end = .
Fixes: 77aed22b48ab ("spl: spl_legacy: Add extra address checks") Reported-by: Francesco Dolcini francesco.dolcini@toradex.com Signed-off-by: Fabio Estevam festevam@denx.de --- Tom,
As 2023.07 is coming in a few days, maybe it is safer to revert 77aed22b48ab in master and then squash 77aed22b48ab + this fix to next.
77aed22b48ab has been applied to 2023.07-rc5, which is too late in the cycle.
common/spl/spl_legacy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c index d34bc5492e8d..095443c63d8d 100644 --- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -19,7 +19,7 @@ static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) { uintptr_t spl_start = (uintptr_t)_start; - uintptr_t spl_end = (uintptr_t)__bss_end; + uintptr_t spl_end = (uintptr_t)_image_binary_end; uintptr_t end = start + size;
if ((start >= spl_start && start < spl_end) ||
On Fri, Jun 30, 2023 at 11:23:11AM -0300, Fabio Estevam wrote:
From: Fabio Estevam festevam@denx.de
Currently, spl_end points to the __bss_end address, which is an external RAM address instead of the end of the SPL text section in the internal RAM.
This causes boot failures on imx6-colibri, for example:
Trying to boot from MMC1 SPL: Image overlaps SPL resetting ...Fix this problem by assigning spl_end to the _image_binary_end, as this symbol properly represents the end of the SPL text section.
From u-boot-spl.map:
.end *(.__end) 0x00000000009121a4 _image_binary_end = .
Fixes: 77aed22b48ab ("spl: spl_legacy: Add extra address checks") Reported-by: Francesco Dolcini francesco.dolcini@toradex.com Signed-off-by: Fabio Estevam festevam@denx.de
Tom,
As 2023.07 is coming in a few days, maybe it is safer to revert 77aed22b48ab in master and then squash 77aed22b48ab + this fix to next.
77aed22b48ab has been applied to 2023.07-rc5, which is too late in the cycle.
I want to see what Marek says about which changes are the most critical for the security issue aspect of this and then figure out what's next.
On Fri, Jun 30, 2023 at 11:23:11AM -0300, Fabio Estevam wrote:
From: Fabio Estevam festevam@denx.de
Currently, spl_end points to the __bss_end address, which is an external RAM address instead of the end of the SPL text section in the internal RAM.
This causes boot failures on imx6-colibri, for example:
Trying to boot from MMC1 SPL: Image overlaps SPL resetting ...Fix this problem by assigning spl_end to the _image_binary_end, as this symbol properly represents the end of the SPL text section.
From u-boot-spl.map:
.end *(.__end) 0x00000000009121a4 _image_binary_end = .
Fixes: 77aed22b48ab ("spl: spl_legacy: Add extra address checks") Reported-by: Francesco Dolcini francesco.dolcini@toradex.com Signed-off-by: Fabio Estevam festevam@denx.de
On mx6cuboxi, Tested-by: Tom Rini trini@konsulko.com
On Fri, Jun 30, 2023 at 11:23 AM Fabio Estevam festevam@gmail.com wrote:
diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c index d34bc5492e8d..095443c63d8d 100644 --- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -19,7 +19,7 @@ static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) { uintptr_t spl_start = (uintptr_t)_start;
uintptr_t spl_end = (uintptr_t)__bss_end;
uintptr_t spl_end = (uintptr_t)_image_binary_end; uintptr_t end = start + size;
I have been thinking more about this and it seems we need to take CONFIG_SPL_SEPARATE_BSS into account.
Should we do this instead?
--- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -19,9 +19,14 @@ static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) { uintptr_t spl_start = (uintptr_t)_start; - uintptr_t spl_end = (uintptr_t)__bss_end; + uintptr_t spl_end; uintptr_t end = start + size;
+ if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS)) + spl_end = (uintptr_t)_image_binary_end; + else + spl_end = (uintptr_t)__bss_end; + if ((start >= spl_start && start < spl_end) || (end > spl_start && end <= spl_end) || (start < spl_start && end >= spl_end) ||
On Fri, Jun 30, 2023 at 3:26 PM Fabio Estevam festevam@gmail.com wrote:
if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS))spl_end = (uintptr_t)_image_binary_end;elsespl_end = (uintptr_t)__bss_end;
I tested this with CONFIG_SPL_SEPARATE_BSS=n and it failed.
The original patch works fine on both cases for me.
On Fri, Jun 30, 2023 at 3:36 PM Fabio Estevam festevam@gmail.com wrote:
I tested this with CONFIG_SPL_SEPARATE_BSS=n and it failed.
Tested this one with CONFIG_SPL_SEPARATE_BSS enabled and disabled:
--- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -18,9 +18,17 @@
static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) { - uintptr_t spl_start = (uintptr_t)_start; - uintptr_t spl_end = (uintptr_t)__bss_end; uintptr_t end = start + size; + uintptr_t spl_start; + uintptr_t spl_end; + + if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS)) { + spl_start = (uintptr_t)_start; + spl_end = (uintptr_t)_image_binary_end; + } else { + spl_start = (uintptr_t)__bss_start; + spl_end = (uintptr_t)__bss_end; + }
Does it look okay?
On Fri, Jun 30, 2023 at 04:08:42PM -0300, Fabio Estevam wrote:
On Fri, Jun 30, 2023 at 3:36 PM Fabio Estevam festevam@gmail.com wrote:
I tested this with CONFIG_SPL_SEPARATE_BSS=n and it failed.
Tested this one with CONFIG_SPL_SEPARATE_BSS enabled and disabled:
--- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -18,9 +18,17 @@
static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) {
uintptr_t spl_start = (uintptr_t)_start;uintptr_t spl_end = (uintptr_t)__bss_end; uintptr_t end = start + size;
uintptr_t spl_start;uintptr_t spl_end;if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS)) {spl_start = (uintptr_t)_start;spl_end = (uintptr_t)_image_binary_end;} else {spl_start = (uintptr_t)__bss_start;spl_end = (uintptr_t)__bss_end;}Does it look okay?
Probably? But have you thrown this at CI, or a world build?
On Fri, Jun 30, 2023 at 4:11 PM Tom Rini trini@konsulko.com wrote:
Probably? But have you thrown this at CI, or a world build?
I threw this at CI, but it failed at test.py qemu_m68k:
https://github.com/u-boot/u-boot/pull/327#partial-pull-merging
On Fri, Jun 30, 2023 at 05:28:57PM -0300, Fabio Estevam wrote:
On Fri, Jun 30, 2023 at 4:11 PM Tom Rini trini@konsulko.com wrote:
Probably? But have you thrown this at CI, or a world build?
I threw this at CI, but it failed at test.py qemu_m68k:
https://github.com/u-boot/u-boot/pull/327#partial-pull-merging
Yeah, just hit re-run when that happens, sadly.
On Fri, Jun 30, 2023 at 6:04 PM Tom Rini trini@konsulko.com wrote:
Yeah, just hit re-run when that happens, sadly.
Ok, only sunxi failed.
If I try:
--- a/arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds +++ b/arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds @@ -37,6 +37,8 @@ SECTIONS __image_copy_end = .; _end = .;
+ _image_binary_end = .; + .bss : { . = ALIGN(4); diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds index 306a4ddf3cd2..0787dfff8fb7 100644 --- a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds +++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds @@ -46,6 +46,8 @@ SECTIONS __image_copy_end = .; _end = .;
+ _image_binary_end = .; + .bss : { . = ALIGN(4);
Then it builds fine.
I am thinking of sending the sunxi lds change as part of a series in v2.
So v2 would become:
1/2: the sunxi lds change above
2/2 the original patch:
--- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -19,7 +19,7 @@ static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) { uintptr_t spl_start = (uintptr_t)_start; - uintptr_t spl_end = (uintptr_t)__bss_end; + uintptr_t spl_end = (uintptr_t)_image_binary_end; uintptr_t end = start + size;
On Fri, Jun 30, 2023 at 08:12:36PM -0300, Fabio Estevam wrote:
On Fri, Jun 30, 2023 at 6:04 PM Tom Rini trini@konsulko.com wrote:
Yeah, just hit re-run when that happens, sadly.
Ok, only sunxi failed.
If I try:
--- a/arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds +++ b/arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds @@ -37,6 +37,8 @@ SECTIONS __image_copy_end = .; _end = .;
_image_binary_end = .;.bss : { . = ALIGN(4);diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds index 306a4ddf3cd2..0787dfff8fb7 100644 --- a/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds +++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl.lds @@ -46,6 +46,8 @@ SECTIONS __image_copy_end = .; _end = .;
_image_binary_end = .;.bss : { . = ALIGN(4);Then it builds fine.
I am thinking of sending the sunxi lds change as part of a series in v2.
So v2 would become:
1/2: the sunxi lds change above
2/2 the original patch:
--- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -19,7 +19,7 @@ static void spl_parse_legacy_validate(uintptr_t start, uintptr_t size) { uintptr_t spl_start = (uintptr_t)_start;
uintptr_t spl_end = (uintptr_t)__bss_end;
uintptr_t spl_end = (uintptr_t)_image_binary_end; uintptr_t end = start + size;
Sounds good.
participants (2)
-
Fabio Estevam -
Tom Rini