[PATCH 2/3] bootstd: Probe bootmeth devices for bootmeths env var

[Simon Glass]

Hi Heinrich, Sam,

On Mon, 13 Jan 2025 at 00:40, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 1/12/25 04:42, Sam Protsenko wrote:
> > Specifying efi_mgr in 'bootmeths' environment variable leads to NULL
> > pointer dereference when 'bootflow scan' is executed, with call trace
> > like this:
> >
> >      priv->fake_dev // NULL pointer dereference
> >      .read_bootflow = efi_mgr_read_bootflow()
> >      bootmeth_get_bootflow()
> >      bootflow_check()
> >      bootflow_scan_first()
> >      do_bootflow_scan()
> >      'bootflow scan -l'
> >
> > That happens because in case when 'bootmeths' env var is defined the
> > bootmeth_efi_mgr driver is not probed, and the memory for its private
> > data isn't allocated by .priv_auto. In case when 'bootmeths' env var is
> > not defined, the std->bootmeth_count is 0, and the execution flow in
> > bootmeth_setup_iter_order() takes "no ordering" path, which in turn runs
> > uclass_get_device_by_seq() -> ... -> device_probe(), so issue isn't
> > present there. But when 'bootmeths' is defined and contains efi_mgr, the
> > std->bootmeth_count > 0, so bootmeth_setup_iter_order() follows the "we
> > have an ordering" path, where devices are not probed. In other words:
> >
> >      'bootmeths' defined           'bootmeths' not defined
> >      --------------------------------------------------------
> >      priv == NULL                    priv != NULL
> >           ^                                ^
> >           |                        device_alloc_priv()
> >       no probe                     device_of_to_plat()
> >           ^                        device_probe()
> >           |                        uclass_get_device_tail()
> >      dev = order[i]                uclass_get_device_by_seq()
> >           ^                                ^
> >           | have an ordering               | no ordering
> >           +----------------+---------------+
> >                            |
> >               bootmeth_setup_iter_order()
> >               bootflow_scan_first()
> >               do_bootflow_scan()
> >
> > Add an explicit device_probe() call in "we have an ordering" case to fix
> > the issue.
> >
> > Fixes: c627cfc14c08 ("bootstd: Allow scanning for global bootmeths separately")
> > Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org>
> > ---
> >   boot/bootmeth-uclass.c | 7 +++++++
> >   1 file changed, 7 insertions(+)

Reviewed-by: Simon Glass <sjg at chromium.org>

> >
> > diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c
> > index ff36da78d5a1..049389403191 100644
> > --- a/boot/bootmeth-uclass.c
> > +++ b/boot/bootmeth-uclass.c
> > @@ -11,6 +11,7 @@
> >   #include <bootmeth.h>
> >   #include <bootstd.h>
> >   #include <dm.h>
> > +#include <dm/device-internal.h>
> >   #include <env_internal.h>
> >   #include <fs.h>
> >   #include <malloc.h>
> > @@ -146,6 +147,12 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global)
> >                               struct bootmeth_uc_plat *ucp;
> >                               bool is_global;
> >
> > +                             ret = device_probe(dev);
> > +                             if (ret) {
> > +                                     ret = log_msg_ret("probe", ret);
> > +                                     goto err_order;
> > +                             }
>
> This would mean that we fail boot if probing one of the boot devices
> fails. Shouldn't we remove the device from the order instead?

It is actually a bootmeth failing to probe which causes problems, not
a bootdev. I think that is catastrophic enough that failing is fine,
at least for now.

Strictly speaking we should not probe the bootmeth until it is
actually used. One way to do this is to probe it in
bootflow_iter_set_dev(), or in its callers, where we already probe the
bootdev.

> > +
> >                               ucp = dev_get_uclass_plat(dev);
> >                               is_global = ucp->flags &
> >                                       BOOTMETHF_GLOBAL;
>

Regards,
Simon