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

[Simon Glass]

Hi Richard,

On Fri, 12 Jul 2024 at 09:11, Richard Weinberger <richard at 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 at 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