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

[Heinrich Schuchardt]

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(+)
>
> 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?

Best regards

Heinrich

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