[PATCH] spl: spl_legacy: Fix spl_end address

[PATCH] spl: spl_legacy: Fix spl_end address

[Fabio Estevam]

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) ||
-- 
2.34.1

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

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>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Fabio Estevam]

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) ||

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Fabio Estevam]

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;
> +       else
> +               spl_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.

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Fabio Estevam]

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?

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Fabio Estevam]

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

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Fabio Estevam]

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;

Re: [PATCH] spl: spl_legacy: Fix spl_end address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]