From 11319e0e2bbf7b3892658d45017cd80c41ad961b Mon Sep 17 00:00:00 2001 From: Sam Protsenko Date: Sat, 11 Jan 2025 21:42:11 -0600 Subject: [PATCH 1/3] bootstd: Fix memleak on errors in bootmeth_setup_iter_order() 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 Reviewed-by: Simon Glass --- 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 014b7588e8d..cb84bb03447 100644 --- a/boot/bootmeth-uclass.c +++ b/boot/bootmeth-uclass.c @@ -135,8 +135,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 *)); @@ -190,8 +192,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) { @@ -202,6 +206,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) From 8da358c0a15a4f538d561602f600b2695fedeb56 Mon Sep 17 00:00:00 2001 From: Sam Protsenko Date: Sat, 11 Jan 2025 21:42:12 -0600 Subject: [PATCH 2/3] bootstd: Probe bootmeth devices for bootmeths env var 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 Reviewed-by: Simon Glass --- boot/bootmeth-uclass.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c index cb84bb03447..57b30bcb513 100644 --- a/boot/bootmeth-uclass.c +++ b/boot/bootmeth-uclass.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -148,6 +149,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; From 8c61fc082e9ae045534dc2f18ed50493adf35e6d Mon Sep 17 00:00:00 2001 From: Sam Protsenko Date: Sat, 11 Jan 2025 21:42:13 -0600 Subject: [PATCH 3/3] bootstd: Fix incorrect struct name in bootmeth_setup_iter_order() 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 --- 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 57b30bcb513..188f6ea1895 100644 --- a/boot/bootmeth-uclass.c +++ b/boot/bootmeth-uclass.c @@ -141,7 +141,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 *)); if (IS_ENABLED(CONFIG_BOOTMETH_GLOBAL)) { for (i = 0; i < count; i++) {