Hi Richard, On Fri, 12 Jul 2024 at 09:11, 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.
Nice commit message. Can you use 'bootstage' as the commit tag? > > Signed-off-by: Richard Weinberger <[email protected]> > --- > 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
