[PATCH v2] bootstage: Fix out-of-bounds read in reloc_bootstage()

[PATCH v2] bootstage: Fix out-of-bounds read in reloc_bootstage()

[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>
---
Changes since v1:
- Pass gd->new_bootstage to bootstage_relocate()
---
 common/board_f.c    | 8 +-------
 common/bootstage.c  | 8 ++++++--
 include/bootstage.h | 4 ++--
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index 29e185137a..21a8944e2b 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -683,13 +683,7 @@ 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();
+		bootstage_relocate(gd->new_bootstage);
 	}
 #endif
 
diff --git a/common/bootstage.c b/common/bootstage.c
index b6c268d9f4..49acc9078a 100644
--- a/common/bootstage.c
+++ b/common/bootstage.c
@@ -54,12 +54,16 @@ struct bootstage_hdr {
 	u32 next_id;		/* Next ID to use for bootstage */
 };
 
-int bootstage_relocate(void)
+int bootstage_relocate(void *to)
 {
-	struct bootstage_data *data = gd->bootstage;
+	struct bootstage_data *data;
 	int i;
 	char *ptr;
 
+	debug("Copying bootstage from %p to %p\n", gd->bootstage, to);
+	memcpy(to, gd->bootstage, sizeof(struct bootstage_data));
+	data = gd->bootstage = to;
+
 	/* Figure out where to relocate the strings to */
 	ptr = (char *)(data + 1);
 
diff --git a/include/bootstage.h b/include/bootstage.h
index f4e77b09d7..57792648c4 100644
--- a/include/bootstage.h
+++ b/include/bootstage.h
@@ -258,7 +258,7 @@ void show_boot_progress(int val);
  * relocation, since memory can be overwritten later.
  * Return: Always returns 0, to indicate success
  */
-int bootstage_relocate(void);
+int bootstage_relocate(void *to);
 
 /**
  * Add a new bootstage record
@@ -395,7 +395,7 @@ static inline ulong bootstage_add_record(enum bootstage_id id,
  * and won't even do that unless CONFIG_SHOW_BOOT_PROGRESS is defined
  */
 
-static inline int bootstage_relocate(void)
+static inline int bootstage_relocate(void *to)
 {
 	return 0;
 }
-- 
2.35.3

Re: [PATCH v2] bootstage: Fix out-of-bounds read in reloc_bootstage()

[Simon Glass]

Hi Richard,

On Wed, 31 Jul 2024 at 10:08, 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.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> Changes since v1:
> - Pass gd->new_bootstage to bootstage_relocate()
> ---
>  common/board_f.c    | 8 +-------
>  common/bootstage.c  | 8 ++++++--
>  include/bootstage.h | 4 ++--
>  3 files changed, 9 insertions(+), 11 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

nit below

> diff --git a/common/board_f.c b/common/board_f.c
> index 29e185137a..21a8944e2b 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -683,13 +683,7 @@ 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();
> +               bootstage_relocate(gd->new_bootstage);
>         }
>  #endif
>
> diff --git a/common/bootstage.c b/common/bootstage.c
> index b6c268d9f4..49acc9078a 100644
> --- a/common/bootstage.c
> +++ b/common/bootstage.c
> @@ -54,12 +54,16 @@ struct bootstage_hdr {
>         u32 next_id;            /* Next ID to use for bootstage */
>  };
>
> -int bootstage_relocate(void)
> +int bootstage_relocate(void *to)
>  {
> -       struct bootstage_data *data = gd->bootstage;
> +       struct bootstage_data *data;
>         int i;
>         char *ptr;
>
> +       debug("Copying bootstage from %p to %p\n", gd->bootstage, to);
> +       memcpy(to, gd->bootstage, sizeof(struct bootstage_data));
> +       data = gd->bootstage = to;

should be a separate line (patman/checkpatch complains)

> +
>         /* Figure out where to relocate the strings to */
>         ptr = (char *)(data + 1);
>
> diff --git a/include/bootstage.h b/include/bootstage.h
> index f4e77b09d7..57792648c4 100644
> --- a/include/bootstage.h
> +++ b/include/bootstage.h
> @@ -258,7 +258,7 @@ void show_boot_progress(int val);
>   * relocation, since memory can be overwritten later.
>   * Return: Always returns 0, to indicate success
>   */
> -int bootstage_relocate(void);
> +int bootstage_relocate(void *to);
>
>  /**
>   * Add a new bootstage record
> @@ -395,7 +395,7 @@ static inline ulong bootstage_add_record(enum bootstage_id id,
>   * and won't even do that unless CONFIG_SHOW_BOOT_PROGRESS is defined
>   */
>
> -static inline int bootstage_relocate(void)
> +static inline int bootstage_relocate(void *to)
>  {
>         return 0;
>  }
> --
> 2.35.3
>

Regards,
Simon

Re: [PATCH v2] bootstage: Fix out-of-bounds read in reloc_bootstage()

[Richard Weinberger]

Simon,

Am Donnerstag, 1. August 2024, 16:42:14 CEST schrieb Simon Glass:
> > +       debug("Copying bootstage from %p to %p\n", gd->bootstage, to);
> > +       memcpy(to, gd->bootstage, sizeof(struct bootstage_data));
> > +       data = gd->bootstage = to;
> 
> should be a separate line (patman/checkpatch complains)

I saw the suggestion of checkpatch.pl but ditched it as matter of taste.
Do you want a v3?

Thanks,
//richard

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

Re: [PATCH v2] bootstage: Fix out-of-bounds read in reloc_bootstage()

[Simon Glass]

Hi Richard,

On Thu, 1 Aug 2024 at 08:48, Richard Weinberger <richard@sigma-star.at> wrote:
>
> Simon,
>
> Am Donnerstag, 1. August 2024, 16:42:14 CEST schrieb Simon Glass:
> > > +       debug("Copying bootstage from %p to %p\n", gd->bootstage, to);
> > > +       memcpy(to, gd->bootstage, sizeof(struct bootstage_data));
> > > +       data = gd->bootstage = to;
> >
> > should be a separate line (patman/checkpatch complains)
>
> I saw the suggestion of checkpatch.pl but ditched it as matter of taste.
> Do you want a v3?

It's OK I suppose. I used to hate that checkpatch.pl rule too, but
have come to get used to it...it avoids hiding the gd-> assignment
that my eye is looking for :-)

Regards,
Simon


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

Re: [PATCH v2] bootstage: Fix out-of-bounds read in reloc_bootstage()

[Tom Rini]

On Wed, 31 Jul 2024 18:07:54 +0200, Richard Weinberger 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.
> 
> [...]

Applied to u-boot/next, thanks!

-- 
Tom