[PATCH 1/3] bootstd: Fix memleak on errors in bootmeth_setup_iter_order()

[Sam Protsenko]

On Mon, Jan 13, 2025 at 1:37 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 1/12/25 04:42, Sam Protsenko wrote:
> > Free memory allocated for 'order' (array of bootmeths) on error paths in
> > bootmeth_setup_iter_order() function.
> >
> > Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately")
> > Fixes: 10d16faa436c ("bootstd: Detect empty bootmeth")
> > Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org>
> > ---
> >   boot/bootmeth-uclass.c | 16 ++++++++++++----
> >   1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c
> > index 5b5fea39b3b3..ff36da78d5a1 100644
> > --- a/boot/bootmeth-uclass.c
> > +++ b/boot/bootmeth-uclass.c
> > @@ -133,8 +133,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global)
> >                * We don't support skipping global bootmeths. Instead, the user
> >                * should omit them from the ordering
> >                */
> > -             if (!include_global)
> > -                     return log_msg_ret("glob", -EPERM);
> > +             if (!include_global) {
> > +                     ret = log_msg_ret("glob", -EPERM);
> > +                     goto err_order;
> > +             }
> >               memcpy(order, std->bootmeth_order,
> >                      count * sizeof(struct bootmeth *));
> >
> > @@ -188,8 +190,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global)
> >               }
> >               count = upto;
> >       }
> > -     if (!count)
> > -             return log_msg_ret("count2", -ENOENT);
> > +     if (!count) {
> > +             ret = log_msg_ret("count2", -ENOENT);
> > +             goto err_order;
> > +     }
> >
> >       if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL) && include_global &&
> >           iter->first_glob_method != -1 && iter->first_glob_method != count) {
> > @@ -200,6 +204,10 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global)
> >       iter->num_methods = count;
> >
> >       return 0;
> > +
> > +err_order:
> > +     free(order);
> > +     return ret;
> >   }
> >
> >   int bootmeth_set_order(const char *order_str)
>
> bootmeth_setup_iter_order() is called when the `boot scan` command is
> executed. The command can be executed multiple times, shouldn't we free
> iter->method_order before reassigning it? Hopefully the field is NULL if
> not initialized.
>

I think it's already taken care of in do_bootflow_scan(), when it's
running bootflow_iter_uninit() in the end of the loop? The relevant
call chain looks like this:

do_bootflow_scan()
    bootflow_scan_first()
        bootflow_iter_init()
            memset(iter, 0)
        bootmeth_setup_iter_order()
            order = calloc()
            iter->method_order = order
    bootflow_scan_next()
    bootflow_iter_uninit()
        free(iter->method_order)

So bootmeth_setup_iter_order() is only called once for each 'bootflow
scan' execution, and it's always called with iter->method_order freed.

> Best regards
>
> Heinrich
>