Hi Richard, On Wed, 31 Jul 2024 at 10:08, Richard Weinberger <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> 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
