* [U-Boot] [PATCH v2 0/1] efi_loader: support BootNext and BootCurrent
@ 2019-03-05 1:58 AKASHI Takahiro
2019-03-05 1:58 ` [U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior AKASHI Takahiro
0 siblings, 1 reply; 6+ messages in thread
From: AKASHI Takahiro @ 2019-03-05 1:58 UTC (permalink / raw)
To: u-boot
This patch was originally posted as a single one, but then was
merged in my "run -e." Now I would like to post it on its own.
With this patch, EFI Boot Manager will handles BootNext and BootCurrent
variable as UEFI specification describes.
Changes in v2 (Mar 5, 2019)
* extract this patch from my "run -e" patch set
* use efi_[get|set]_variable instead of 'rs->*'
* add EFI_CALL
* cosmetic changes
AKASHI Takahiro (1):
efi_loader: bootmgr: support BootNext and BootCurrent variable
behavior
lib/efi_loader/efi_bootmgr.c | 40 ++++++++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 4 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior
2019-03-05 1:58 [U-Boot] [PATCH v2 0/1] efi_loader: support BootNext and BootCurrent AKASHI Takahiro
@ 2019-03-05 1:58 ` AKASHI Takahiro
2019-03-05 19:38 ` Heinrich Schuchardt
0 siblings, 1 reply; 6+ messages in thread
From: AKASHI Takahiro @ 2019-03-05 1:58 UTC (permalink / raw)
To: u-boot
See UEFI v2.7, section 3.1.2 for details of the specification.
With efidebug command, you can run any EFI boot option as follows:
=> 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 ...)
=> env print -e
<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 | 40 ++++++++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 417016102b48..1575c5c09e46 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -141,6 +141,7 @@ 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",
@@ -151,6 +152,16 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
if (ret != EFI_SUCCESS)
goto error;
+ attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS;
+ size = sizeof(n);
+ ret = EFI_CALL(efi_set_variable(
+ L"BootCurrent",
+ (efi_guid_t *)&efi_global_variable_guid,
+ attributes, size, &n));
+ if (ret != EFI_SUCCESS)
+ goto error;
+
printf("Booting: %ls\n", lo.label);
efi_dp_split_file_path(lo.file_path, device_path, file_path);
}
@@ -162,21 +173,42 @@ error:
}
/*
- * Attempt to load, in the order specified by BootOrder EFI variable, the
- * available load-options, finding and returning the first one that can
- * be loaded successfully.
+ * Attempt to load from BootNext or in the order specified by BootOrder
+ * EFI variable, the available load-options, finding and returning
+ * the first one that can be loaded successfully.
*/
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;
bs = systab.boottime;
rs = systab.runtime;
+ /* BootNext */
+ size = sizeof(bootnext);
+ ret = EFI_CALL(efi_get_variable(L"BootNext",
+ (efi_guid_t *)&efi_global_variable_guid,
+ NULL, &size, &bootnext));
+ if (ret == EFI_SUCCESS) {
+ /* delete BootNext */
+ ret = EFI_CALL(efi_set_variable(
+ L"BootNext",
+ (efi_guid_t *)&efi_global_variable_guid,
+ 0, 0, &bootnext));
+ if (ret == EFI_SUCCESS) {
+ image = try_load_entry(bootnext,
+ device_path, file_path);
+ if (image)
+ goto error;
+ }
+ }
+
+ /* BootOrder */
bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
if (!bootorder) {
printf("BootOrder not defined\n");
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior
2019-03-05 1:58 ` [U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior AKASHI Takahiro
@ 2019-03-05 19:38 ` Heinrich Schuchardt
2019-03-06 0:17 ` AKASHI Takahiro
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2019-03-05 19:38 UTC (permalink / raw)
To: u-boot
On 3/5/19 2:58 AM, AKASHI Takahiro wrote:
> See UEFI v2.7, section 3.1.2 for details of the specification.
>
> With efidebug command, you can run any EFI boot option as follows:
> => 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 ...)
> => env print -e
> <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>
Please, use scripts/get_maintainer.pl to determine the correct
recipients. You missed Alex's new email address.
> ---
> lib/efi_loader/efi_bootmgr.c | 40 ++++++++++++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 417016102b48..1575c5c09e46 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -141,6 +141,7 @@ 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",
> @@ -151,6 +152,16 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> if (ret != EFI_SUCCESS)
> goto error;
>
> + attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS;
> + size = sizeof(n);
> + ret = EFI_CALL(efi_set_variable(
> + L"BootCurrent",
> + (efi_guid_t *)&efi_global_variable_guid,
> + attributes, size, &n));
> + if (ret != EFI_SUCCESS)
> + goto error;
> +
> printf("Booting: %ls\n", lo.label);
> efi_dp_split_file_path(lo.file_path, device_path, file_path);
> }
> @@ -162,21 +173,42 @@ error:
> }
>
> /*
> - * Attempt to load, in the order specified by BootOrder EFI variable, the
> - * available load-options, finding and returning the first one that can
> - * be loaded successfully.
> + * Attempt to load from BootNext or in the order specified by BootOrder
> + * EFI variable, the available load-options, finding and returning
> + * the first one that can be loaded successfully.
> */
> void *efi_bootmgr_load(struct efi_device_path **device_path,
> struct efi_device_path **file_path)
> {
> - uint16_t *bootorder;
> + u16 bootnext, *bootorder;
bootnext has enough space for the terminating \n. That is way too small.
You want to call efi_get_variable() twice. Get the buffer size needed in
the first round. malloc() a buffer. Then actually read the variable.
Finally free() the buffer.
> efi_uintn_t size;
> void *image = NULL;
> int i, num;
> + efi_status_t ret;
>
> bs = systab.boottime;
> rs = systab.runtime;
>
> + /* BootNext */
> + size = sizeof(bootnext);
> + ret = EFI_CALL(efi_get_variable(L"BootNext",
> + (efi_guid_t *)&efi_global_variable_guid,
> + NULL, &size, &bootnext));
> + if (ret == EFI_SUCCESS) {
The expected value of ret for an existing variable of size > 0 is
EFI_BUFFER_TOO_SMALL.
> + /* delete BootNext */
> + ret = EFI_CALL(efi_set_variable(
> + L"BootNext",
> + (efi_guid_t *)&efi_global_variable_guid,
> + 0, 0, &bootnext));
> + if (ret == EFI_SUCCESS) {
Why would loading the boot entry depend on the return value here?
> + image = try_load_entry(bootnext,
Best regards
Heinrich
> + device_path, file_path);
> + if (image)
> + goto error;
> + }
> + }
> +
> + /* BootOrder */
> bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> if (!bootorder) {
> printf("BootOrder not defined\n");
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior
2019-03-05 19:38 ` Heinrich Schuchardt
@ 2019-03-06 0:17 ` AKASHI Takahiro
2019-03-06 4:21 ` Heinrich Schuchardt
0 siblings, 1 reply; 6+ messages in thread
From: AKASHI Takahiro @ 2019-03-06 0:17 UTC (permalink / raw)
To: u-boot
On Tue, Mar 05, 2019 at 08:38:40PM +0100, Heinrich Schuchardt wrote:
> On 3/5/19 2:58 AM, AKASHI Takahiro wrote:
> > See UEFI v2.7, section 3.1.2 for details of the specification.
> >
> > With efidebug command, you can run any EFI boot option as follows:
> > => 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 ...)
> > => env print -e
> > <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>
>
> Please, use scripts/get_maintainer.pl to determine the correct
> recipients. You missed Alex's new email address.
Okay.
> > ---
> > lib/efi_loader/efi_bootmgr.c | 40 ++++++++++++++++++++++++++++++++----
> > 1 file changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 417016102b48..1575c5c09e46 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -141,6 +141,7 @@ 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",
> > @@ -151,6 +152,16 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> > if (ret != EFI_SUCCESS)
> > goto error;
> >
> > + attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS;
> > + size = sizeof(n);
> > + ret = EFI_CALL(efi_set_variable(
> > + L"BootCurrent",
> > + (efi_guid_t *)&efi_global_variable_guid,
> > + attributes, size, &n));
> > + if (ret != EFI_SUCCESS)
> > + goto error;
> > +
> > printf("Booting: %ls\n", lo.label);
> > efi_dp_split_file_path(lo.file_path, device_path, file_path);
> > }
> > @@ -162,21 +173,42 @@ error:
> > }
> >
> > /*
> > - * Attempt to load, in the order specified by BootOrder EFI variable, the
> > - * available load-options, finding and returning the first one that can
> > - * be loaded successfully.
> > + * Attempt to load from BootNext or in the order specified by BootOrder
> > + * EFI variable, the available load-options, finding and returning
> > + * the first one that can be loaded successfully.
> > */
> > void *efi_bootmgr_load(struct efi_device_path **device_path,
> > struct efi_device_path **file_path)
> > {
> > - uint16_t *bootorder;
> > + u16 bootnext, *bootorder;
>
> bootnext has enough space for the terminating \n. That is way too small.
>
> You want to call efi_get_variable() twice. Get the buffer size needed in
> the first round. malloc() a buffer. Then actually read the variable.
> Finally free() the buffer.
No.
"BootNext" is always a 16-bit integer, and "bootnext" is a u16 variable,
not a pointer.
So we don't need to call get_variable() twice. That is why I didn't use
get_var() here. I believe that you seem to like "code efficiency."
> > efi_uintn_t size;
> > void *image = NULL;
> > int i, num;
> > + efi_status_t ret;
> >
> > bs = systab.boottime;
> > rs = systab.runtime;
> >
> > + /* BootNext */
> > + size = sizeof(bootnext);
> > + ret = EFI_CALL(efi_get_variable(L"BootNext",
> > + (efi_guid_t *)&efi_global_variable_guid,
> > + NULL, &size, &bootnext));
> > + if (ret == EFI_SUCCESS) {
>
> The expected value of ret for an existing variable of size > 0 is
> EFI_BUFFER_TOO_SMALL.
>
> > + /* delete BootNext */
> > + ret = EFI_CALL(efi_set_variable(
> > + L"BootNext",
> > + (efi_guid_t *)&efi_global_variable_guid,
> > + 0, 0, &bootnext));
> > + if (ret == EFI_SUCCESS) {
>
> Why would loading the boot entry depend on the return value here?
Deleting "BootNex" is required by UEFI spec.
In addition, if we fail to delete it, we will see the same application
start again when executing boot manager next time.
Thanks,
-Takahiro Akashi
> > + image = try_load_entry(bootnext,
>
> Best regards
>
> Heinrich
>
> > + device_path, file_path);
> > + if (image)
> > + goto error;
> > + }
> > + }
> > +
> > + /* BootOrder */
> > bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> > if (!bootorder) {
> > printf("BootOrder not defined\n");
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior
2019-03-06 0:17 ` AKASHI Takahiro
@ 2019-03-06 4:21 ` Heinrich Schuchardt
2019-03-08 0:51 ` AKASHI Takahiro
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2019-03-06 4:21 UTC (permalink / raw)
To: u-boot
On 3/6/19 1:17 AM, AKASHI Takahiro wrote:
> On Tue, Mar 05, 2019 at 08:38:40PM +0100, Heinrich Schuchardt wrote:
>> On 3/5/19 2:58 AM, AKASHI Takahiro wrote:
>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>
>>> With efidebug command, you can run any EFI boot option as follows:
>>> => 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 ...)
>>> => env print -e
>>> <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>
>>
>> Please, use scripts/get_maintainer.pl to determine the correct
>> recipients. You missed Alex's new email address.
>
> Okay.
>
>>> ---
>>> lib/efi_loader/efi_bootmgr.c | 40 ++++++++++++++++++++++++++++++++----
>>> 1 file changed, 36 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 417016102b48..1575c5c09e46 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -141,6 +141,7 @@ 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",
>>> @@ -151,6 +152,16 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>>> if (ret != EFI_SUCCESS)
>>> goto error;
>>>
>>> + attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> + EFI_VARIABLE_RUNTIME_ACCESS;
>>> + size = sizeof(n);
>>> + ret = EFI_CALL(efi_set_variable(
>>> + L"BootCurrent",
>>> + (efi_guid_t *)&efi_global_variable_guid,
>>> + attributes, size, &n));
>>> + if (ret != EFI_SUCCESS)
>>> + goto error;
>>> +
>>> printf("Booting: %ls\n", lo.label);
>>> efi_dp_split_file_path(lo.file_path, device_path, file_path);
>>> }
>>> @@ -162,21 +173,42 @@ error:
>>> }
>>>
>>> /*
>>> - * Attempt to load, in the order specified by BootOrder EFI variable, the
>>> - * available load-options, finding and returning the first one that can
>>> - * be loaded successfully.
>>> + * Attempt to load from BootNext or in the order specified by BootOrder
>>> + * EFI variable, the available load-options, finding and returning
>>> + * the first one that can be loaded successfully.
>>> */
>>> void *efi_bootmgr_load(struct efi_device_path **device_path,
>>> struct efi_device_path **file_path)
>>> {
>>> - uint16_t *bootorder;
>>> + u16 bootnext, *bootorder;
>>
>> bootnext has enough space for the terminating \n. That is way too small.
>>
>> You want to call efi_get_variable() twice. Get the buffer size needed in
>> the first round. malloc() a buffer. Then actually read the variable.
>> Finally free() the buffer.
>
> No.
> "BootNext" is always a 16-bit integer, and "bootnext" is a u16 variable,
> not a pointer.
> So we don't need to call get_variable() twice. That is why I didn't use
> get_var() here. I believe that you seem to like "code efficiency."
I see.
>
>>> efi_uintn_t size;
>>> void *image = NULL;
>>> int i, num;
>>> + efi_status_t ret;
>>>
>>> bs = systab.boottime;
>>> rs = systab.runtime;
>>>
>>> + /* BootNext */
>>> + size = sizeof(bootnext);
>>> + ret = EFI_CALL(efi_get_variable(L"BootNext",
>>> + (efi_guid_t *)&efi_global_variable_guid,
>>> + NULL, &size, &bootnext));
>>> + if (ret == EFI_SUCCESS) {
As we expect the variable to have size 2, we should check the size field
too.
>>
>> The expected value of ret for an existing variable of size > 0 is
>> EFI_BUFFER_TOO_SMALL.
Now let's assume that the variable has been created with the wrong size
(e.g. using `env set -e`). In that case we should either try to delete
it or write an error message or both.
>>
>>> + /* delete BootNext */
>>> + ret = EFI_CALL(efi_set_variable(
>>> + L"BootNext",
>>> + (efi_guid_t *)&efi_global_variable_guid,
>>> + 0, 0, &bootnext));
>>> + if (ret == EFI_SUCCESS) {
>>
>> Why would loading the boot entry depend on the return value here?
>
> Deleting "BootNex" is required by UEFI spec.
> In addition, if we fail to delete it, we will see the same application
> start again when executing boot manager next time.
>
BootNext is a non-volatile variable. Once we have a non-volatile backend
for variables the call may fail because the the NV storage is not
writable. Another case leading to an error would be the variable having
been created with EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS
(which we do not yet support).
Should this stop the boot process? If yes, we need at least an error
message. But I would propose that if the NV storage is not writable we
continue booting after writing a debug message.
Best regards
Heinrich
> Thanks,
> -Takahiro Akashi
>
>>> + image = try_load_entry(bootnext,
>>
>> Best regards
>>
>> Heinrich
>>
>>> + device_path, file_path);
>>> + if (image)
>>> + goto error;
>>> + }
>>> + }
>>> +
>>> + /* BootOrder */
>>> bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
>>> if (!bootorder) {
>>> printf("BootOrder not defined\n");
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior
2019-03-06 4:21 ` Heinrich Schuchardt
@ 2019-03-08 0:51 ` AKASHI Takahiro
0 siblings, 0 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2019-03-08 0:51 UTC (permalink / raw)
To: u-boot
On Wed, Mar 06, 2019 at 05:21:54AM +0100, Heinrich Schuchardt wrote:
> On 3/6/19 1:17 AM, AKASHI Takahiro wrote:
> > On Tue, Mar 05, 2019 at 08:38:40PM +0100, Heinrich Schuchardt wrote:
> >> On 3/5/19 2:58 AM, AKASHI Takahiro wrote:
> >>> See UEFI v2.7, section 3.1.2 for details of the specification.
> >>>
> >>> With efidebug command, you can run any EFI boot option as follows:
> >>> => 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 ...)
> >>> => env print -e
> >>> <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>
> >>
> >> Please, use scripts/get_maintainer.pl to determine the correct
> >> recipients. You missed Alex's new email address.
> >
> > Okay.
> >
> >>> ---
> >>> lib/efi_loader/efi_bootmgr.c | 40 ++++++++++++++++++++++++++++++++----
> >>> 1 file changed, 36 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index 417016102b48..1575c5c09e46 100644
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -141,6 +141,7 @@ 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",
> >>> @@ -151,6 +152,16 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >>> if (ret != EFI_SUCCESS)
> >>> goto error;
> >>>
> >>> + attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> + EFI_VARIABLE_RUNTIME_ACCESS;
> >>> + size = sizeof(n);
> >>> + ret = EFI_CALL(efi_set_variable(
> >>> + L"BootCurrent",
> >>> + (efi_guid_t *)&efi_global_variable_guid,
> >>> + attributes, size, &n));
> >>> + if (ret != EFI_SUCCESS)
> >>> + goto error;
> >>> +
> >>> printf("Booting: %ls\n", lo.label);
> >>> efi_dp_split_file_path(lo.file_path, device_path, file_path);
> >>> }
> >>> @@ -162,21 +173,42 @@ error:
> >>> }
> >>>
> >>> /*
> >>> - * Attempt to load, in the order specified by BootOrder EFI variable, the
> >>> - * available load-options, finding and returning the first one that can
> >>> - * be loaded successfully.
> >>> + * Attempt to load from BootNext or in the order specified by BootOrder
> >>> + * EFI variable, the available load-options, finding and returning
> >>> + * the first one that can be loaded successfully.
> >>> */
> >>> void *efi_bootmgr_load(struct efi_device_path **device_path,
> >>> struct efi_device_path **file_path)
> >>> {
> >>> - uint16_t *bootorder;
> >>> + u16 bootnext, *bootorder;
> >>
> >> bootnext has enough space for the terminating \n. That is way too small.
> >>
> >> You want to call efi_get_variable() twice. Get the buffer size needed in
> >> the first round. malloc() a buffer. Then actually read the variable.
> >> Finally free() the buffer.
> >
> > No.
> > "BootNext" is always a 16-bit integer, and "bootnext" is a u16 variable,
> > not a pointer.
> > So we don't need to call get_variable() twice. That is why I didn't use
> > get_var() here. I believe that you seem to like "code efficiency."
>
> I see.
>
> >
> >>> efi_uintn_t size;
> >>> void *image = NULL;
> >>> int i, num;
> >>> + efi_status_t ret;
> >>>
> >>> bs = systab.boottime;
> >>> rs = systab.runtime;
> >>>
> >>> + /* BootNext */
> >>> + size = sizeof(bootnext);
> >>> + ret = EFI_CALL(efi_get_variable(L"BootNext",
> >>> + (efi_guid_t *)&efi_global_variable_guid,
> >>> + NULL, &size, &bootnext));
> >>> + if (ret == EFI_SUCCESS) {
>
> As we expect the variable to have size 2, we should check the size field
> too.
Here we see EFI_SUCCESS only if the size of variable is 1 or 2.
In case of size of 1, it's not in correct format, but I think
that it's safe and acceptable.
So basically I don't think that we need check the size.
(except for a message below)
> >>
> >> The expected value of ret for an existing variable of size > 0 is
> >> EFI_BUFFER_TOO_SMALL.
>
> Now let's assume that the variable has been created with the wrong size
> (e.g. using `env set -e`). In that case we should either try to delete
> it or write an error message or both.
I simply want to write a message.
> >>
> >>> + /* delete BootNext */
> >>> + ret = EFI_CALL(efi_set_variable(
> >>> + L"BootNext",
> >>> + (efi_guid_t *)&efi_global_variable_guid,
> >>> + 0, 0, &bootnext));
> >>> + if (ret == EFI_SUCCESS) {
> >>
> >> Why would loading the boot entry depend on the return value here?
> >
> > Deleting "BootNex" is required by UEFI spec.
> > In addition, if we fail to delete it, we will see the same application
> > start again when executing boot manager next time.
> >
>
> BootNext is a non-volatile variable. Once we have a non-volatile backend
> for variables the call may fail because the the NV storage is not
> writable. Another case leading to an error would be the variable having
> been created with EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS
> (which we do not yet support).
Since we have no idea how we will manage "non-volatile" variables
in UEFI U-Boot, it is still early to discuss any behavior based on
not-agreed assumptions.
For example, if NV storage is not writable, we will probably not be
able to define BootNext variable.
> Should this stop the boot process? If yes, we need at least an error
> message. But I would propose that if the NV storage is not writable we
> continue booting after writing a debug message.
I'm afraid that this can be used as a DoS attack.
Anyhow, we should be "conservative" :)
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> > Thanks,
> > -Takahiro Akashi
> >
> >>> + image = try_load_entry(bootnext,
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> + device_path, file_path);
> >>> + if (image)
> >>> + goto error;
> >>> + }
> >>> + }
> >>> +
> >>> + /* BootOrder */
> >>> bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> >>> if (!bootorder) {
> >>> printf("BootOrder not defined\n");
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-08 0:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-05 1:58 [U-Boot] [PATCH v2 0/1] efi_loader: support BootNext and BootCurrent AKASHI Takahiro
2019-03-05 1:58 ` [U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior AKASHI Takahiro
2019-03-05 19:38 ` Heinrich Schuchardt
2019-03-06 0:17 ` AKASHI Takahiro
2019-03-06 4:21 ` Heinrich Schuchardt
2019-03-08 0:51 ` AKASHI Takahiro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.