[PATCH] bootstd: Avoid freeing a non-allocated buffer

[Shantur Rathore]

Hi Simon,


> [please can you avoid top-posting as it makes it had for people to read later]
>

Sure thing.

> There is a pending series there which I haven't got to yet.
>
I can add that change to my bootmeth_efi dhcp fixes patch series if you want.

> BTW, for this patch we need a test which covers EFI on sandbox,
> perhaps running the helloworld program. The current bootstd testing
> does not extend into the efi_loader code, so there is really no
> coverage of the code in efi_exit_boot_services() from bootstd.

I am not sure how sandbox works.
I was able to test my setup which was crashing and was fixed after
these changes.

Regard,
Shantur

>
> Regards,
> Simon
>
>
> >
> > Kind regards,
> > Shantur
> >
> > On Thu, Nov 16, 2023 at 1:35 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > EFI applications can be very large and thus used to cause boot failures
> > > when malloc() space was exhausted.
> > >
> > > A recent changed fixed this by using the kernel_addr_r environment var
> > > as the address of the buffer. However, it still frees the buffer when
> > > the bootflow is discarded.
> > >
> > > Fix this by introducing a flag to indicate whether the buffer was
> > > allocated, or not.
> > >
> > > Note that kernel_addr_r is not the last word here. It might be better
> > > to use lmb to place images. But there is a lot of refactoring to do
> > > before we can remove the environment variables. The distro scripts rely
> > > on them so it is safe for bootstd to do so too.
> > >
> > > Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
> > >
> > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > Reported by: Simon Glass <sjg at chromium.org>
> > > Reported by: Shantur Rathore <i at shantur.com>
> > > ---
> > >
> > >  boot/bootflow.c     | 3 ++-
> > >  boot/bootmeth_efi.c | 1 +
> > >  include/bootflow.h  | 5 ++++-
> > >  3 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > index 6922e7e0c4e7..1ea2966ae9a5 100644
> > > --- a/boot/bootflow.c
> > > +++ b/boot/bootflow.c
> > > @@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
> > >         free(bflow->name);
> > >         free(bflow->subdir);
> > >         free(bflow->fname);
> > > -       free(bflow->buf);
> > > +       if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
> > > +               free(bflow->buf);
> > >         free(bflow->os_name);
> > >         free(bflow->fdt_fname);
> > >         free(bflow->bootmeth_priv);
> > > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> > > index ae936c8daa18..9ba7734911e1 100644
> > > --- a/boot/bootmeth_efi.c
> > > +++ b/boot/bootmeth_efi.c
> > > @@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow, ulong addr)
> > >         if (ret)
> > >                 return log_msg_ret("read", ret);
> > >         bflow->buf = map_sysmem(addr, bflow->size);
> > > +       bflow->flags |= BOOTFLOWF_STATIC_BUF;
> > >
> > >         set_efi_bootdev(desc, bflow);
> > >
> > > diff --git a/include/bootflow.h b/include/bootflow.h
> > > index 44d3741eacae..fede8f22a2b8 100644
> > > --- a/include/bootflow.h
> > > +++ b/include/bootflow.h
> > > @@ -43,9 +43,12 @@ enum bootflow_state_t {
> > >   *     and it is using the prior-stage FDT, which is the U-Boot control FDT.
> > >   *     This is only possible with the EFI bootmeth (distro-efi) and only when
> > >   *     CONFIG_OF_HAS_PRIOR_STAGE is enabled
> > > + * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set, rather
> > > + *     than being allocated by malloc().
> > >   */
> > >  enum bootflow_flags_t {
> > >         BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
> > > +       BOOTFLOWF_STATIC_BUF    = 1 << 1,
> > >  };
> > >
> > >  /**
> > > @@ -72,7 +75,7 @@ enum bootflow_flags_t {
> > >   * @fname: Filename of bootflow file (allocated)
> > >   * @logo: Logo to display for this bootflow (BMP format)
> > >   * @logo_size: Size of the logo in bytes
> > > - * @buf: Bootflow file contents (allocated)
> > > + * @buf: Bootflow file contents (allocated unless @flags & BOOTFLOWF_STATIC_BUF)
> > >   * @size: Size of bootflow file in bytes
> > >   * @err: Error number received (0 if OK)
> > >   * @os_name: Name of the OS / distro being booted, or NULL if not known
> > > --
> > > 2.43.0.rc0.421.g78406f8d94-goog
> > >