| 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;
Hi Heinrich, Sam, On Mon, 13 Jan 2025 at 00:40, Heinrich Schuchardt <xypron.glpk@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@linaro.org> > > --- > > boot/bootmeth-uclass.c | 7 +++++++ > > 1 file changed, 7 insertions(+) Reviewed-by: Simon Glass <sjg@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
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(+)