On Wed, Feb 27, 2019 at 07:14:10AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 6:47 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
See UEFI v2.7, section 3.1.2 for details of the specification.
With my efitool command, you can try as the following: => efi boot add 1 SHELL ... => efi boot add 2 HELLO ... => efi boot order 1 2 => efi bootmgr (starting SHELL ...) => efi boot next 2 => efi bootmgr (starting HELLO ...) => efi dumpvar <snip ...> BootCurrent: {boot,run}(blob) 00000000: 02 00 .. BootOrder: {boot,run}(blob) 00000000: 01 00 02 00 ....
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a095df3f540b..6c5303736dc6 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, efi_deserialize_load_option(&lo, load_option);
if (lo.attributes & LOAD_OPTION_ACTIVE) {
u32 attributes;efi_status_t ret;
debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);
attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS;size = sizeof(n);ret = rs->set_variable(L"BootCurrent",(efi_guid_t *)&efi_global_variable_guid,Use EFI_CALL().
Okay But as I said somewhere else, it's quite annoying to me that some efi_xxx requires EFI_CALL(), and others not. There should have been consistent naming rules.
We started with having separate functions like efi_allocate_pages_ext() and efi_allocate_pages(). Then Rob Clark came along and introduced EFI_CALL() in a095aadffa96 and I stopped creating _ext() functions.
When running with DEBUG 1 it sometimes is helpful to see which function is calling which other and where errors are originally reported.
But I am open to changes in this area.
Instead of dereferencing you could directly call efi_set_variable().
Yeah, given that this code is under lib/efi_loader, it may be natural to use efi_set_variable(). But existing get_var() uses the same style of coding.
Do you want to change all of the call sites including get_var()?
Calling efi_set_variable() directly uses less bytes of code than rs->get_variable() which makes it preferable.
So is your answer yes, or no?
I have seen that iPXE modifies system->boottime to intercept system calls. The same could be done by an EFI driver to the runtime vectors.
In the light of your work on secure boot I think we should not allow an EFI driver to intercept the reading and changing of variables here.
We should also rethink it for efidebug.c
I'm not sure about your concern here, but no doubt efidebug should be disabled on production system with secure boot.
Thanks, -Takahiro Akashi
Best regards
Heinrich
attributes, size, &n);if (ret != EFI_SUCCESS)goto error;ret = efi_load_image_from_path(lo.file_path, &image);
if (ret != EFI_SUCCESS)
@@ -173,16 +183,38 @@ error: void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path) {
- uint16_t *bootorder;
u16 bootnext, *bootorder; efi_uintn_t size; void *image = NULL; int i, num;
efi_status_t ret;
__efi_entry_check();
bs = systab.boottime; rs = systab.runtime;
/* get BootNext */
size = sizeof(bootnext);
ret = rs->get_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,NULL, &size, &bootnext);You could call efi_get_variable() directly instead of dereferencing rs. But anyway you have to use EFI_CALL().
Ditto
- if (!bootnext)
goto run_list;Goto is acceptable for error handling. But otherwise I would rather avoid it.
Okay with another indentation.
- /* delete BootNext */
- ret = rs->set_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,0, 0, &bootnext);EFI_CALL().
Thanks, -Takahiro Akashi
Best regards
Heinrich
- if (ret != EFI_SUCCESS)
goto error;- image = try_load_entry(bootnext, device_path, file_path);
- if (image)
goto error;+run_list:
- /* BootOrder */ bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) goto error;