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

[Alexander Graf]

On 27.03.19 14:10, Heinrich Schuchardt wrote:
> 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.


For secure boot, the device tree must not come from the user, correct.
However, for secure boot, the user must never get access to the U-Boot
shell either, as that would allow access to arbitrary memory
modification commands. So in a way, we can assume that $fdtaddr is
trusted I guess?

At the same time, it means that the boot command (and thus environment)
is trusted too, so there is no need to prohibit passing a DT location, no?

Sorry for mentioning this so late :)


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


How would you do a signature check on the DT?


Alex


>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at 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 */
>>