[PATCH 0/4] boot: fix crash in bootflow menu with EFI BOOTMGR support + typos

bootflow menu currently crashes U-Boot with a NULL pointer dereference because bootflow->dev is NULL for global bootmeths (such as EFI BOOTMGR). Therefore, let's check if the bootflow is associated with a global bootmeth before trying to make it part of the menu.
While this makes U-Boot not crash anymore, bootflow menu doesn't work for me (I have never had a happy path with it, but I haven't actually tried it before today :) ) and this was basically just implemented following Simon's suggestion sent over IRC. No clue if this is enough or just a quick band-aid patch.
This also fixes typos in multiple places.
Signed-off-by: Quentin Schulz quentin.schulz@cherry.de --- Quentin Schulz (4): cmd: fix typo in CMD_BOOTMETH help text boot: fix typos in help text of Kconfig configs doc: bootstd: fix typos boot: bootflow_menu: fix crash for EFI BOOTMGR global bootmeth
boot/Kconfig | 22 +++++++++++----------- boot/bootflow_menu.c | 7 +++++++ cmd/Kconfig | 2 +- doc/develop/bootstd.rst | 50 ++++++++++++++++++++++++------------------------- include/bootflow.h | 3 ++- 5 files changed, 46 insertions(+), 38 deletions(-) --- base-commit: 1ebd659cf020843fd8e8ef90d85a66941cbab6ec change-id: 20240612-bootflow-efi-crash-c52449ef643e
Best regards,

From: Quentin Schulz quentin.schulz@cherry.de
It's bootmeths and not bootmethds.
Signed-off-by: Quentin Schulz quentin.schulz@cherry.de --- cmd/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index b026439c773..57929d4462d 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -308,7 +308,7 @@ config CMD_BOOTMETH depends on BOOTSTD default y if BOOTSTD_FULL help - Support listing available bootmethds (methods used to boot an + Support listing available bootmeths (methods used to boot an Operating System), as well as selecting the order that the bootmeths are used.

On Wed, 12 Jun 2024 at 08:58, Quentin Schulz foss+uboot@0leil.net wrote:
From: Quentin Schulz quentin.schulz@cherry.de
It's bootmeths and not bootmethds.
Signed-off-by: Quentin Schulz quentin.schulz@cherry.de
cmd/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org

From: Quentin Schulz quentin.schulz@cherry.de
This fixes a handful of typos in various help texts in Kconfig configs.
Signed-off-by: Quentin Schulz quentin.schulz@cherry.de --- boot/Kconfig | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig index 6f3096c15a6..de277d2ba50 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -423,7 +423,7 @@ config SPL_BOOTSTD depends on SPL && SPL_DM && SPL_OF_CONTROL && SPL_BLK default y if VPL help - This enables standard boot in SPL. This is neeeded so that VBE + This enables standard boot in SPL. This is needed so that VBE (Verified Boot for Embedded) can be used, since it depends on standard boot. It is enabled by default since the main purpose of VPL is to handle the firmware part of VBE. @@ -433,7 +433,7 @@ config VPL_BOOTSTD depends on VPL && VPL_DM && VPL_OF_CONTROL && VPL_BLK default y help - This enables standard boot in SPL. This is neeeded so that VBE + This enables standard boot in SPL. This is needed so that VBE (Verified Boot for Embedded) can be used, since it depends on standard boot. It is enabled by default since the main purpose of VPL is to handle the firmware part of VBE. @@ -449,7 +449,7 @@ config BOOTSTD_FULL - bootdev, bootmeth commands - extra features in the bootflow command - support for selecting the ordering of bootmeths ("bootmeth order") - - support for selecting the ordering of bootdevs using the devicetree + - support for selecting the ordering of bootdevs using the Device Tree as well as the "boot_targets" environment variable
config BOOTSTD_DEFAULTS @@ -481,7 +481,7 @@ config BOOTSTD_PROG default y help Enable this to provide a board_run_command() function which can boot - a systen without using commands. If the boot fails, then U-Boot will + a system without using commands. If the boot fails, then U-Boot will panic.
Note: This currently has many limitations and is not a useful booting @@ -517,7 +517,7 @@ config BOOTMETH_EXTLINUX bootdevs look for a 'extlinux/extlinux.conf' on each filesystem they scan.
- The specification for this filed is here: + The specification for this file is here:
https://uapi-group.org/specifications/specs/boot_loader_specification/
@@ -576,7 +576,7 @@ config BOOTMETH_VBE select EVENT help Enables support for VBE boot. This is a standard boot method which - supports selection of various firmware components, seleciton of an OS to + supports selection of various firmware components, selection of an OS to boot as well as updating these using fwupd.
config BOOTMETH_DISTRO @@ -593,7 +593,7 @@ config SPL_BOOTMETH_VBE default y if VPL help Enables support for VBE boot. This is a standard boot method which - supports selection of various firmware components, seleciton of an OS to + supports selection of various firmware components, selection of an OS to boot as well as updating these using fwupd.
config VPL_BOOTMETH_VBE @@ -603,7 +603,7 @@ config VPL_BOOTMETH_VBE default y help Enables support for VBE boot. This is a standard boot method which - supports selection of various firmware components, seleciton of an OS to + supports selection of various firmware components, selection of an OS to boot as well as updating these using fwupd.
if BOOTMETH_VBE @@ -748,7 +748,7 @@ if MEASURED_BOOT bool "Measure the devicetree image" default y if MEASURED_BOOT help - On some platforms, the devicetree is not static as it may contain + On some platforms, the Device Tree is not static as it may contain random MAC addresses or other such data that changes each boot. Therefore, it should not be measured into the TPM. In that case, disable the measurement here. @@ -1303,7 +1303,7 @@ config AUTOBOOT_PROMPT
Note that this define is used as the (only) argument to a printf() call, so it may contain '%' format specifications, - provided that it also includes, sepearated by commas exactly + provided that it also includes, separated by commas exactly like in a printf statement, the required arguments. It is the responsibility of the user to select only such arguments that are valid in the given context. @@ -1402,7 +1402,7 @@ config AUTOBOOT_STOP_STR_SHA256 help This option adds the feature to only stop the autobooting, and therefore boot into the U-Boot prompt, when the input - string / password matches a values that is encypted via + string / password matches a values that is encrypted via a SHA256 hash and saved in the environment variable "bootstopkeysha256". If the value in that variable includes a ":", the portion prior to the ":" will be treated

On Wed, 12 Jun 2024 at 08:58, Quentin Schulz foss+uboot@0leil.net wrote:
From: Quentin Schulz quentin.schulz@cherry.de
This fixes a handful of typos in various help texts in Kconfig configs.
Signed-off-by: Quentin Schulz quentin.schulz@cherry.de
boot/Kconfig | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

From: Quentin Schulz quentin.schulz@cherry.de
This fixes a few syntactic issues as well as typos and grammar.
Signed-off-by: Quentin Schulz quentin.schulz@cherry.de --- doc/develop/bootstd.rst | 50 ++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/doc/develop/bootstd.rst b/doc/develop/bootstd.rst index a07a72581e7..34631089ae0 100644 --- a/doc/develop/bootstd.rst +++ b/doc/develop/bootstd.rst @@ -39,7 +39,7 @@ Bootflow
A bootflow is a file that describes how to boot a distro. Conceptually there can be different formats for that file but at present U-Boot only supports the -BootLoaderSpec_ format. which looks something like this:: +BootLoaderSpec_ format which looks something like this::
menu autoboot Welcome to Fedora-Workstation-armhfp-31-1.9. Automatic boot in # second{,s}. Press a key for options. menu title Fedora-Workstation-armhfp-31-1.9 Boot Options. @@ -52,7 +52,7 @@ BootLoaderSpec_ format. which looks something like this:: initrd /initramfs-5.3.7-301.fc31.armv7hl.img
As you can see it specifies a kernel, a ramdisk (initrd) and a directory from -which to load devicetree files. The details are described in distro_bootcmd_. +which to load Device Tree files. The details are described in distro_bootcmd_.
The bootflow is provided by the distro. It is not part of U-Boot. U-Boot's job is simply to interpret the file and carry out the instructions. This allows @@ -85,7 +85,7 @@ Bootmeth --------
Once the list of filesystems is provided, how does U-Boot find the bootflow -files in these filesystems. That is the job of bootmeth. Each boot method has +files in these filesystems? That is the job of bootmeth. Each boot method has its own way of doing this.
For example, the distro bootmeth simply looks through the provided filesystem @@ -106,7 +106,7 @@ they scan a lot of devices. Boot process ------------
-U-Boot tries to use the 'lazy init' approach whereever possible and distro boot +U-Boot tries to use the 'lazy init' approach wherever possible and distro boot is no exception. The algorithm is::
while (get next bootdev) @@ -174,13 +174,13 @@ the same way as setting this variable. Bootdev uclass --------------
-The bootdev uclass provides an simple API call to obtain a bootflows from a +The bootdev uclass provides a simple API call to obtain a bootflow from a device::
int bootdev_get_bootflow(struct udevice *dev, struct bootflow_iter *iter, struct bootflow *bflow);
-This takes a iterator which indicates the bootdev, partition and bootmeth to +This takes an iterator which indicates the bootdev, partition and bootmeth to use. It returns a bootflow. This is the core of the bootdev implementation. The bootdev drivers that implement this differ depending on the media they are reading from, but each is responsible for returning a valid bootflow if @@ -188,7 +188,7 @@ available.
A helper called `bootdev_find_in_blk()` makes it fairly easy to implement this function for each media device uclass, in a few lines of code. For many types -ot bootdevs, the `get_bootflow` member can be NULL, indicating that the default +of bootdevs, the `get_bootflow` member can be NULL, indicating that the default handler is used. This is called `default_get_bootflow()` and it only works with block devices.
@@ -196,7 +196,7 @@ block devices. Bootdev drivers ---------------
-A bootdev driver is typically fairly simple. Here is one for mmc:: +A bootdev driver is typically fairly simple. Here is one for MMC::
static int mmc_bootdev_bind(struct udevice *dev) { @@ -328,7 +328,7 @@ or::
Here, `eth_bootdev` is the name of the Ethernet bootdev driver and `dev` -is the ethernet device. This function is safe to call even if standard boot is +is the Ethernet device. This function is safe to call even if standard boot is not enabled, since it does nothing in that case. It can be added to all uclasses which implement suitable media.
@@ -340,7 +340,7 @@ Standard boot requires a single instance of the bootstd device to make things work. This includes global information about the state of standard boot. See `struct bootstd_priv` for this structure, accessed with `bootstd_get_priv()`.
-Within the devicetree, if you add bootmeth devices, they should be children of +Within the Device Tree, if you add bootmeth devices, they should be children of the bootstd device. See `arch/sandbox/dts/test.dts` for an example of this.
@@ -349,12 +349,12 @@ the bootstd device. See `arch/sandbox/dts/test.dts` for an example of this. Automatic devices -----------------
-It is possible to define all the required devices in the devicetree manually, +It is possible to define all the required devices in the Device Tree manually, but it is not necessary. The bootstd uclass includes a `dm_scan_other()` function which creates the bootstd device if not found. If no bootmeth devices are found at all, it creates one for each available bootmeth driver.
-If your devicetree has any bootmeth device it must have all of them that you +If your Device Tree has any bootmeth device it must have all of them that you want to use, since no bootmeth devices will be created automatically in that case.
@@ -363,8 +363,8 @@ Using devicetree ----------------
If a bootdev is complicated or needs configuration information, it can be -added to the devicetree as a child of the media device. For example, imagine a -bootdev which reads a bootflow from SPI flash. The devicetree fragment might +added to the Device Tree as a child of the media device. For example, imagine a +bootdev which reads a bootflow from SPI flash. The Device Tree fragment might look like this::
spi@0 { @@ -398,7 +398,7 @@ Standard boot is enabled with `CONFIG_BOOTSTD`. Each bootmeth has its own CONFIG option also. For example, `CONFIG_BOOTMETH_EXTLINUX` enables support for booting from a disk using an `extlinux.conf` file.
-To enable all feature sof standard boot, use `CONFIG_BOOTSTD_FULL`. This +To enable all features of standard boot, use `CONFIG_BOOTSTD_FULL`. This includes the full set of commands, more error messages when things go wrong and bootmeth ordering with the bootmeths environment variable.
@@ -492,9 +492,9 @@ Theory of operation This describes how standard boot progresses through to booting an operating system.
-To start. all the necessary devices must be bound, including bootstd, which +To start, all the necessary devices must be bound, including bootstd, which provides the top-level `struct bootstd_priv` containing optional configuration -information. The bootstd device is also holds the various lists used while +information. The bootstd device also holds the various lists used while scanning. This step is normally handled automatically by driver model, as described in `Automatic Devices`_.
@@ -504,7 +504,7 @@ those bootdevs. So, all up, we need a single bootstd device, one or more bootdev devices and one or more bootmeth devices.
Once these are ready, typically a `bootflow scan` command is issued. This kicks -of the iteration process, which involves hunting for bootdevs and looking +off the iteration process, which involves hunting for bootdevs and looking through the bootdevs and their partitions one by one to find bootflows.
Iteration is kicked off using `bootflow_scan_first()`. @@ -526,7 +526,7 @@ Then the iterator is set up to according to the parameters given:
- If `label` indicates a numeric bootdev number (e.g. "2") then `BOOTFLOW_METHF_SINGLE_DEV` is set. In this case, moving to the next bootdev - simple stops, since there is only one. No hunters are used. + simply stops, since there is only one. No hunters are used. - If `label` indicates a particular media device (e.g. "mmc1") then `BOOTFLOWIF_SINGLE_MEDIA` is set. In this case, moving to the next bootdev processes just the children of the media device. Hunters are used, in this @@ -554,7 +554,7 @@ bootdev and disturb the original ordering.
Next, the ordering of bootmeths is determined, by `bootmeth_setup_iter_order()`. By default the ordering is again by sequence number, i.e. the `/aliases` node, -or failing that the order in the devicetree. But the `bootmeth order` command +or failing that the order in the Device Tree. But the `bootmeth order` command or `bootmeths` environment variable can be used to set up an ordering. If that has been done, the ordering is in `struct bootstd_priv`, so that ordering is simply copied into the iterator. Either way, the `method_order` array it set up, @@ -652,12 +652,12 @@ valid bootflow is found early on. With `bootflow scan -b`, that causes the bootflow to be immediately booted. Assuming it is successful, the iteration never completes.
-Also note that the iterator hold the **current** combination being considered. +Also note that the iterator holds the **current** combination being considered. So when `iter_incr()` is called, it increments to the next one and returns it, the new **current** combination.
Note also the `err` field in `struct bootflow_iter`. This is normally 0 and has -thus has no effect on `iter_inc()`. But if it is non-zero, signalling an error, +thus no effect on `iter_inc()`. But if it is non-zero, signalling an error, it indicates to the iterator what it should do when called. It can force moving to the next partition, or bootdev, for example. The special values `BF_NO_MORE_PARTS` and `BF_NO_MORE_DEVICES` handle this. When `iter_incr` sees @@ -675,7 +675,7 @@ So what happens inside of `bootflow_check()`? It simply calls the uclass method `bootdev_get_bootflow()` to ask the bootdev to return a bootflow. It passes the iterator to the bootdev method, so that function knows what we are talking about. At first, the bootflow is set up in the state `BOOTFLOWST_BASE`, -with just the `method` and `dev` intiialised. But the bootdev may fill in more, +with just the `method` and `dev` initialised. But the bootdev may fill in more, e.g. updating the state, depending on what it finds. For global bootmeths the `bootmeth_get_bootflow()` function is called instead of `bootdev_get_bootflow()`. @@ -733,12 +733,12 @@ bootflow is handled by the bootmeth driver for that bootflow. In the case of extlinux boot, this parses and processes the `extlinux.conf` file that was read. See `extlinux_boot()` for how that works. The processing may involve reading additional files, which is handled by the `read_file()` method, which is -`extlinux_read_file()` in this case. All bootmethds should support reading +`extlinux_read_file()` in this case. All bootmeths should support reading files, since the bootflow is typically only the basic instructions and does not include the operating system itself, ramdisk, device tree, etc.
The vast majority of the bootstd code is concerned with iterating through -partitions on bootdevs and using bootmethds to find bootflows. +partitions on bootdevs and using bootmeths to find bootflows.
How about bootdevs which are not block devices? They are handled by the same methods as above, but with a different implementation. For example, the bootmeth

On Wed, 12 Jun 2024 at 08:58, Quentin Schulz foss+uboot@0leil.net wrote:
From: Quentin Schulz quentin.schulz@cherry.de
This fixes a few syntactic issues as well as typos and grammar.
Signed-off-by: Quentin Schulz quentin.schulz@cherry.de
doc/develop/bootstd.rst | 50 ++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-)
Thank you!
Reviewed-by: Simon Glass sjg@chromium.org

From: Quentin Schulz quentin.schulz@cherry.de
The global bootmeths don't set the dev in bootflow struct which means the dev_get_parent(bflow->dev) triggers a NULL-pointer dereference and crash U-Boot.
So before trying to handle a bootflow, check that the associated bootmeth isn't global, otherwise skip it.
Suggested-by: Simon Glass sjg@chromium.org Signed-off-by: Quentin Schulz quentin.schulz@cherry.de --- boot/bootflow_menu.c | 7 +++++++ include/bootflow.h | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/boot/bootflow_menu.c b/boot/bootflow_menu.c index 16f9cd8f8ca..70f1f728b72 100644 --- a/boot/bootflow_menu.c +++ b/boot/bootflow_menu.c @@ -10,6 +10,7 @@
#include <common.h> #include <bootflow.h> +#include <bootmeth.h> #include <bootstd.h> #include <cli.h> #include <dm.h> @@ -77,6 +78,7 @@ int bootflow_menu_new(struct expo **expp) last_bootdev = NULL; for (ret = bootflow_first_glob(&bflow), i = 0; !ret && i < 36; ret = bootflow_next_glob(&bflow), i++) { + struct bootmeth_uc_plat *ucp; char str[2], *label, *key; uint preview_id; bool add_gap; @@ -84,6 +86,11 @@ int bootflow_menu_new(struct expo **expp) if (bflow->state != BOOTFLOWST_READY) continue;
+ /* No media to show for BOOTMETHF_GLOBAL bootmeths */ + ucp = dev_get_uclass_plat(bflow->method); + if (ucp->flags & BOOTMETHF_GLOBAL) + continue; + *str = i < 10 ? '0' + i : 'A' + i - 10; str[1] = '\0'; key = strdup(str); diff --git a/include/bootflow.h b/include/bootflow.h index 080ee850122..6affc5e1a4f 100644 --- a/include/bootflow.h +++ b/include/bootflow.h @@ -63,7 +63,8 @@ enum bootflow_flags_t { * * @bm_node: Points to siblings in the same bootdev * @glob_node: Points to siblings in the global list (all bootdev) - * @dev: Bootdev device which produced this bootflow + * @dev: Bootdev device which produced this bootflow, NULL for flows created by + * BOOTMETHF_GLOBAL bootmeths * @blk: Block device which contains this bootflow, NULL if this is a network * device or sandbox 'host' device * @part: Partition number (0 for whole device)

On Wed, 12 Jun 2024 at 08:58, Quentin Schulz foss+uboot@0leil.net wrote:
From: Quentin Schulz quentin.schulz@cherry.de
The global bootmeths don't set the dev in bootflow struct which means the dev_get_parent(bflow->dev) triggers a NULL-pointer dereference and crash U-Boot.
So before trying to handle a bootflow, check that the associated bootmeth isn't global, otherwise skip it.
Suggested-by: Simon Glass sjg@chromium.org Signed-off-by: Quentin Schulz quentin.schulz@cherry.de
boot/bootflow_menu.c | 7 +++++++ include/bootflow.h | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org

On Wed, 12 Jun 2024 16:58:45 +0200, Quentin Schulz wrote:
bootflow menu currently crashes U-Boot with a NULL pointer dereference because bootflow->dev is NULL for global bootmeths (such as EFI BOOTMGR). Therefore, let's check if the bootflow is associated with a global bootmeth before trying to make it part of the menu.
While this makes U-Boot not crash anymore, bootflow menu doesn't work for me (I have never had a happy path with it, but I haven't actually tried it before today :) ) and this was basically just implemented following Simon's suggestion sent over IRC. No clue if this is enough or just a quick band-aid patch.
[...]
Applied to u-boot/next, thanks!
participants (3)
-
Quentin Schulz
-
Simon Glass
-
Tom Rini