On Mon, Jan 13, 2025 at 1:37 AM Heinrich Schuchardt <xypron.g...@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.protse...@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
>
