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.protsenko@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.

Best regards

Heinrich

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

On 1/12/25 04:42, Sam Protsenko wrote:

There is no such thing as struct bootmeth, it's probably a typo. This issue doesn't affect the execution as it's a pointer, and pointer sizes are the same for all data types. But it can be confusing, so make it struct udevice, as it should be.

Fixes: a950d31abe98 ("bootstd: Add the bootmeth uclass and helpers") Signed-off-by: Sam Protsenko semen.protsenko@linaro.org

---

boot/bootmeth-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c index 049389403191..2496e8c1d8a8 100644 --- a/boot/bootmeth-uclass.c +++ b/boot/bootmeth-uclass.c @@ -139,7 +139,7 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global) goto err_order; } memcpy(order, std->bootmeth_order,

•        count * sizeof(struct bootmeth *));
  

•        count * sizeof(struct udevice *));
  

I found this description of the field: @bootmeth_order: List of bootmeth devices to use, in order, NULL-terminated

As the list is NULL terminated, shouldn't we copy the NULL value, i.e.

(count + 1) * sizeof(struct udevice *)

so that we can still identify the end of the list?

Best regards

Heinrich

if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL)) {
	for (i = 0; i < count; i++) {