[PATCH] reloc_bootstage: Fix out-of-bounds read

[PATCH] reloc_bootstage: Fix out-of-bounds read

[Richard Weinberger]

bootstage_get_size() returns the total size of the data structure
including associated records.
When copying from gd->bootstage, only the allocation size of gd->bootstage
must be used. Otherwise too much memory is copied.

This bug caused no harm so far because gd->new_bootstage is always
large enough and reading beyond the allocation length of gd->bootstage
caused no problem due to the U-Boot memory layout.

Fix by using the correct size and perform the initial copy directly
in bootstage_relocate() to have the whole relocation process in the
same function.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 common/board_f.c   | 6 ------
 common/bootstage.c | 7 ++++++-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index 039d6d712d..f4d87692b9 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -683,12 +683,6 @@ static int reloc_bootstage(void)
 	if (gd->flags & GD_FLG_SKIP_RELOC)
 		return 0;
 	if (gd->new_bootstage) {
-		int size = bootstage_get_size();
-
-		debug("Copying bootstage from %p to %p, size %x\n",
-		      gd->bootstage, gd->new_bootstage, size);
-		memcpy(gd->new_bootstage, gd->bootstage, size);
-		gd->bootstage = gd->new_bootstage;
 		bootstage_relocate();
 	}
 #endif
diff --git a/common/bootstage.c b/common/bootstage.c
index 0e6d80718f..aea5a318df 100644
--- a/common/bootstage.c
+++ b/common/bootstage.c
@@ -58,10 +58,15 @@ struct bootstage_hdr {
 
 int bootstage_relocate(void)
 {
-	struct bootstage_data *data = gd->bootstage;
+	struct bootstage_data *data;
 	int i;
 	char *ptr;
 
+	debug("Copying bootstage from %p to %p\n", gd->bootstage,
+	      gd->new_bootstage);
+	memcpy(gd->new_bootstage, gd->bootstage, sizeof(struct bootstage_data));
+	data = gd->bootstage = gd->new_bootstage;
+
 	/* Figure out where to relocate the strings to */
 	ptr = (char *)(data + 1);
 
-- 
2.35.3

Re: [PATCH] reloc_bootstage: Fix out-of-bounds read

[Simon Glass]

Hi Richard,

On Fri, 12 Jul 2024 at 09:11, Richard Weinberger <richard@nod.at> wrote:
>
> bootstage_get_size() returns the total size of the data structure
> including associated records.
> When copying from gd->bootstage, only the allocation size of gd->bootstage
> must be used. Otherwise too much memory is copied.
>
> This bug caused no harm so far because gd->new_bootstage is always
> large enough and reading beyond the allocation length of gd->bootstage
> caused no problem due to the U-Boot memory layout.
>
> Fix by using the correct size and perform the initial copy directly
> in bootstage_relocate() to have the whole relocation process in the
> same function.

Nice commit message.

Can you use 'bootstage' as the commit tag?

>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  common/board_f.c   | 6 ------
>  common/bootstage.c | 7 ++++++-
>  2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/common/board_f.c b/common/board_f.c
> index 039d6d712d..f4d87692b9 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -683,12 +683,6 @@ static int reloc_bootstage(void)
>         if (gd->flags & GD_FLG_SKIP_RELOC)
>                 return 0;
>         if (gd->new_bootstage) {
> -               int size = bootstage_get_size();
> -
> -               debug("Copying bootstage from %p to %p, size %x\n",
> -                     gd->bootstage, gd->new_bootstage, size);
> -               memcpy(gd->new_bootstage, gd->bootstage, size);
> -               gd->bootstage = gd->new_bootstage;
>                 bootstage_relocate();
>         }
>  #endif
> diff --git a/common/bootstage.c b/common/bootstage.c
> index 0e6d80718f..aea5a318df 100644
> --- a/common/bootstage.c
> +++ b/common/bootstage.c
> @@ -58,10 +58,15 @@ struct bootstage_hdr {
>
>  int bootstage_relocate(void)
>  {
> -       struct bootstage_data *data = gd->bootstage;
> +       struct bootstage_data *data;
>         int i;
>         char *ptr;
>
> +       debug("Copying bootstage from %p to %p\n", gd->bootstage,
> +             gd->new_bootstage);
> +       memcpy(gd->new_bootstage, gd->bootstage, sizeof(struct bootstage_data));

I would like to have the relocation addresses in board_f like with
other relocations, so it is easy to see what is happening, in one
file. So how about passing the old address to bootstage_relocate() so
it doesn't need to access gd->new_bootstage ?

> +       data = gd->bootstage = gd->new_bootstage;
> +
>         /* Figure out where to relocate the strings to */
>         ptr = (char *)(data + 1);
>
> --
> 2.35.3
>

Regards,
Simon

Re: [PATCH] reloc_bootstage: Fix out-of-bounds read

[Richard Weinberger]

Simon,

Am Samstag, 13. Juli 2024, 17:13:50 CEST schrieb Simon Glass:
> Can you use 'bootstage' as the commit tag?

Sure.

> > +       debug("Copying bootstage from %p to %p\n", gd->bootstage,
> > +             gd->new_bootstage);
> > +       memcpy(gd->new_bootstage, gd->bootstage, sizeof(struct bootstage_data));
> 
> I would like to have the relocation addresses in board_f like with
> other relocations, so it is easy to see what is happening, in one
> file. So how about passing the old address to bootstage_relocate() so
> it doesn't need to access gd->new_bootstage ?

You mean passing the *new* address?

Thanks,
//richard

-- 
​​​​​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y

Re: [PATCH] reloc_bootstage: Fix out-of-bounds read

[Simon Glass]

Hi Richard,

On Mon, 29 Jul 2024 at 15:57, Richard Weinberger <richard@sigma-star.at> wrote:
>
> Simon,
>
> Am Samstag, 13. Juli 2024, 17:13:50 CEST schrieb Simon Glass:
> > Can you use 'bootstage' as the commit tag?
>
> Sure.
>
> > > +       debug("Copying bootstage from %p to %p\n", gd->bootstage,
> > > +             gd->new_bootstage);
> > > +       memcpy(gd->new_bootstage, gd->bootstage, sizeof(struct bootstage_data));
> >
> > I would like to have the relocation addresses in board_f like with
> > other relocations, so it is easy to see what is happening, in one
> > file. So how about passing the old address to bootstage_relocate() so
> > it doesn't need to access gd->new_bootstage ?
>
> You mean passing the *new* address?

Yes, sorry.

REgards,
Simon