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

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