| Message ID | 20250112034213.13153-3-semen.protsenko@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | bootstd: Fix efi_mgr usage in bootmeths env var | expand |
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@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;
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; + } + ucp = dev_get_uclass_plat(dev); is_global = ucp->flags & BOOTMETHF_GLOBAL;
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@linaro.org> --- boot/bootmeth-uclass.c | 7 +++++++ 1 file changed, 7 insertions(+)