Re: [U-Boot] [RFC v2 11/11] cmd: add efibootmgr command

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:

Add CONFIG_CMD_STANDALONE_EFIBOOTMGR. With this patch, EFI boot manager can be kicked in by a standalone command, efibootmgr.

I miss your comment form 0/11 here:

* When we will support "secure boot" in the future, EFI Boot Manager is expected to be invoked as a standalone command without any arguments to mitigate security surfaces.

Simply requiring that environment variable $fdtaddr, which is under user control, is used for loading the device tree does not offer any security at all.

For secure boot I would expect that the device tree has to be part of a signed binary and that that signature is checked.

If the user passes in a device tree that should be okay if it has the required signature.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/Kconfig | 8 ++++++++ cmd/bootefi.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig index 4bcc5c45579d..aa27b21956b3 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -224,6 +224,14 @@ config CMD_BOOTEFI help Boot an EFI image from memory.

+config CMD_STANDALONE_EFIBOOTMGR

• bool "Enable EFI Boot Manager as a standalone command"
• depends on CMD_BOOTEFI
• default n
• help

•      With this option enabled, EFI Boot Manager can be invoked
  

•  as a standalone command.
  

config CMD_BOOTEFI_HELLO_COMPILE bool "Compile a standard EFI hello world binary for testing" depends on CMD_BOOTEFI && !CPU_V7M && !SANDBOX diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 17dccd718008..2a2ff4034831 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -635,3 +635,38 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path) bootefi_image_path = NULL; } }

+#ifdef CONFIG_CMD_STANDALONE_EFIBOOTMGR +static int do_efibootmgr_cmd(cmd_tbl_t *cmdtp, int flag, int argc,

• 	     char * const argv[])
  

+{

• char *fdt_opt;
• int ret;
• if (argc != 1)

• return CMD_RET_USAGE;
  

It is unclear why you do not allow the user to pass the location of the device tree as a parameter.

• fdt_opt = env_get("fdtaddr");

You are looking at some environment variable $fdtaddr while most boards use $fdt_addr_r as preferred address to load the device tree.

Best regards

Heinrich

• if (!fdt_opt)> + fdt_opt = env_get("fdtcontroladdr");
• ret = do_efibootmgr(fdt_opt);
• free(fdt_opt);
• return ret;

+}

+#ifdef CONFIG_SYS_LONGHELP +static char efibootmgr_help_text[] =

• "(no arguments)\n"
• " - Launch EFI boot manager and execute EFI application\n"
• " based on BootNext and BootOrder variables\n";

+#endif

+U_BOOT_CMD(

• efibootmgr, 1, 0, do_efibootmgr_cmd,
• "Launch EFI boot manager",
• efibootmgr_help_text

+); +#endif /* CONFIG_CMD_STANDALONE_EFIBOOTMGR */