* [U-Boot] [PATCH v2 1/1] efi_loader: unload applications upon Exit()
@ 2019-05-07 19:13 Heinrich Schuchardt
2019-05-07 23:59 ` Takahiro Akashi
0 siblings, 1 reply; 5+ messages in thread
From: Heinrich Schuchardt @ 2019-05-07 19:13 UTC (permalink / raw)
To: u-boot
Implement unloading of images in the Exit() boot services:
* unload images that are not yet started,
* unload started applications,
* unload drivers returning an error.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
Images that are no yet started can be unloaded by calling Exit().
In this case they are not the current image. Move the test for
current down in the code.
A started driver that called Exit() should still be considered a
started image. Exit cannot be called by another image afterwards,
cf. UEFI SCT 2.6 (2017), 3.5.1 Exit(), 5.1.4.5.8 - 5.1.4.5.10.
---
include/efi_loader.h | 1 +
lib/efi_loader/efi_boottime.c | 36 +++++++++++++++++++++++++------
lib/efi_loader/efi_image_loader.c | 2 ++
3 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 3b50cd28ef..4e4cffa799 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -234,6 +234,7 @@ struct efi_loaded_image_obj {
struct jmp_buf_data exit_jmp;
EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
struct efi_system_table *st);
+ u16 image_type;
};
/**
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 0385883ded..1ea96dab6c 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -13,6 +13,7 @@
#include <linux/libfdt_env.h>
#include <u-boot/crc.h>
#include <bootm.h>
+#include <pe.h>
#include <watchdog.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
* image protocol.
*/
efi_status_t ret;
- void *info;
+ struct efi_loaded_image *loaded_image_protocol;
struct efi_loaded_image_obj *image_obj =
(struct efi_loaded_image_obj *)image_handle;
@@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
exit_data_size, exit_data);
/* Check parameters */
- if (image_handle != current_image)
- goto out;
ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
- &info, NULL, NULL,
+ (void **)&loaded_image_protocol,
+ NULL, NULL,
EFI_OPEN_PROTOCOL_GET_PROTOCOL));
- if (ret != EFI_SUCCESS)
+ if (ret != EFI_SUCCESS) {
+ ret = EFI_INVALID_PARAMETER;
goto out;
+ }
+
+ /* Unloading of unstarted images */
+ switch (image_obj->header.type) {
+ case EFI_OBJECT_TYPE_STARTED_IMAGE:
+ break;
+ case EFI_OBJECT_TYPE_LOADED_IMAGE:
+ efi_delete_image(image_obj, loaded_image_protocol);
+ ret = EFI_SUCCESS;
+ goto out;
+ default:
+ /* Handle does not refer to loaded image */
+ ret = EFI_INVALID_PARAMETER;
+ goto out;
+ }
+ /* A started image can only be unloaded it is the last one started. */
+ if (image_handle != current_image) {
+ ret = EFI_INVALID_PARAMETER;
+ goto out;
+ }
/* Exit data is only foreseen in case of failure. */
if (exit_status != EFI_SUCCESS) {
@@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
if (ret != EFI_SUCCESS)
EFI_PRINT("%s: out of memory\n", __func__);
}
+ if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
+ exit_status != EFI_SUCCESS)
+ efi_delete_image(image_obj, loaded_image_protocol);
/* Make sure entry/exit counts for EFI world cross-overs match */
EFI_EXIT(exit_status);
@@ -2837,7 +2861,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
panic("EFI application exited");
out:
- return EFI_EXIT(EFI_INVALID_PARAMETER);
+ return EFI_EXIT(ret);
}
/**
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index f8092b6202..13541cfa7a 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -273,6 +273,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
image_base = opt->ImageBase;
efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
+ handle->image_type = opt->Subsystem;
efi_reloc = efi_alloc(virt_size,
loaded_image_info->image_code_type);
if (!efi_reloc) {
@@ -288,6 +289,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
image_base = opt->ImageBase;
efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
+ handle->image_type = opt->Subsystem;
efi_reloc = efi_alloc(virt_size,
loaded_image_info->image_code_type);
if (!efi_reloc) {
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 1/1] efi_loader: unload applications upon Exit()
2019-05-07 19:13 [U-Boot] [PATCH v2 1/1] efi_loader: unload applications upon Exit() Heinrich Schuchardt
@ 2019-05-07 23:59 ` Takahiro Akashi
2019-05-08 0:59 ` Heinrich Schuchardt
0 siblings, 1 reply; 5+ messages in thread
From: Takahiro Akashi @ 2019-05-07 23:59 UTC (permalink / raw)
To: u-boot
On Tue, May 07, 2019 at 09:13:24PM +0200, Heinrich Schuchardt wrote:
> Implement unloading of images in the Exit() boot services:
>
> * unload images that are not yet started,
> * unload started applications,
> * unload drivers returning an error.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
> Images that are no yet started can be unloaded by calling Exit().
> In this case they are not the current image. Move the test for
> current down in the code.
>
> A started driver that called Exit() should still be considered a
> started image. Exit cannot be called by another image afterwards,
> cf. UEFI SCT 2.6 (2017), 3.5.1 Exit(), 5.1.4.5.8 - 5.1.4.5.10.
> ---
> include/efi_loader.h | 1 +
> lib/efi_loader/efi_boottime.c | 36 +++++++++++++++++++++++++------
> lib/efi_loader/efi_image_loader.c | 2 ++
> 3 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 3b50cd28ef..4e4cffa799 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -234,6 +234,7 @@ struct efi_loaded_image_obj {
> struct jmp_buf_data exit_jmp;
> EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
> struct efi_system_table *st);
> + u16 image_type;
> };
>
> /**
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 0385883ded..1ea96dab6c 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -13,6 +13,7 @@
> #include <linux/libfdt_env.h>
> #include <u-boot/crc.h>
> #include <bootm.h>
> +#include <pe.h>
> #include <watchdog.h>
>
> DECLARE_GLOBAL_DATA_PTR;
> @@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> * image protocol.
> */
> efi_status_t ret;
> - void *info;
> + struct efi_loaded_image *loaded_image_protocol;
> struct efi_loaded_image_obj *image_obj =
> (struct efi_loaded_image_obj *)image_handle;
>
> @@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> exit_data_size, exit_data);
>
> /* Check parameters */
> - if (image_handle != current_image)
> - goto out;
> ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
> - &info, NULL, NULL,
> + (void **)&loaded_image_protocol,
> + NULL, NULL,
> EFI_OPEN_PROTOCOL_GET_PROTOCOL));
> - if (ret != EFI_SUCCESS)
> + if (ret != EFI_SUCCESS) {
> + ret = EFI_INVALID_PARAMETER;
> goto out;
> + }
> +
> + /* Unloading of unstarted images */
> + switch (image_obj->header.type) {
> + case EFI_OBJECT_TYPE_STARTED_IMAGE:
> + break;
> + case EFI_OBJECT_TYPE_LOADED_IMAGE:
> + efi_delete_image(image_obj, loaded_image_protocol);
> + ret = EFI_SUCCESS;
> + goto out;
> + default:
> + /* Handle does not refer to loaded image */
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + /* A started image can only be unloaded it is the last one started. */
> + if (image_handle != current_image) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
>
> /* Exit data is only foreseen in case of failure. */
> if (exit_status != EFI_SUCCESS) {
> @@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> if (ret != EFI_SUCCESS)
> EFI_PRINT("%s: out of memory\n", __func__);
> }
> + if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
> + exit_status != EFI_SUCCESS)
> + efi_delete_image(image_obj, loaded_image_protocol);
No change around efi_delete_image() and "goto" above?
-Takahiro Akashi
> /* Make sure entry/exit counts for EFI world cross-overs match */
> EFI_EXIT(exit_status);
> @@ -2837,7 +2861,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>
> panic("EFI application exited");
> out:
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + return EFI_EXIT(ret);
> }
>
> /**
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index f8092b6202..13541cfa7a 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -273,6 +273,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
> IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
> image_base = opt->ImageBase;
> efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> + handle->image_type = opt->Subsystem;
> efi_reloc = efi_alloc(virt_size,
> loaded_image_info->image_code_type);
> if (!efi_reloc) {
> @@ -288,6 +289,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
> IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
> image_base = opt->ImageBase;
> efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> + handle->image_type = opt->Subsystem;
> efi_reloc = efi_alloc(virt_size,
> loaded_image_info->image_code_type);
> if (!efi_reloc) {
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 1/1] efi_loader: unload applications upon Exit()
2019-05-07 23:59 ` Takahiro Akashi
@ 2019-05-08 0:59 ` Heinrich Schuchardt
2019-05-08 1:08 ` Takahiro Akashi
0 siblings, 1 reply; 5+ messages in thread
From: Heinrich Schuchardt @ 2019-05-08 0:59 UTC (permalink / raw)
To: u-boot
On 5/8/19 1:59 AM, Takahiro Akashi wrote:
> On Tue, May 07, 2019 at 09:13:24PM +0200, Heinrich Schuchardt wrote:
>> Implement unloading of images in the Exit() boot services:
>>
>> * unload images that are not yet started,
>> * unload started applications,
>> * unload drivers returning an error.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2
>> Images that are no yet started can be unloaded by calling Exit().
>> In this case they are not the current image. Move the test for
>> current down in the code.
>>
>> A started driver that called Exit() should still be considered a
>> started image. Exit cannot be called by another image afterwards,
>> cf. UEFI SCT 2.6 (2017), 3.5.1 Exit(), 5.1.4.5.8 - 5.1.4.5.10.
>> ---
>> include/efi_loader.h | 1 +
>> lib/efi_loader/efi_boottime.c | 36 +++++++++++++++++++++++++------
>> lib/efi_loader/efi_image_loader.c | 2 ++
>> 3 files changed, 33 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 3b50cd28ef..4e4cffa799 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -234,6 +234,7 @@ struct efi_loaded_image_obj {
>> struct jmp_buf_data exit_jmp;
>> EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
>> struct efi_system_table *st);
>> + u16 image_type;
>> };
>>
>> /**
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 0385883ded..1ea96dab6c 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -13,6 +13,7 @@
>> #include <linux/libfdt_env.h>
>> #include <u-boot/crc.h>
>> #include <bootm.h>
>> +#include <pe.h>
>> #include <watchdog.h>
>>
>> DECLARE_GLOBAL_DATA_PTR;
>> @@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>> * image protocol.
>> */
>> efi_status_t ret;
>> - void *info;
>> + struct efi_loaded_image *loaded_image_protocol;
>> struct efi_loaded_image_obj *image_obj =
>> (struct efi_loaded_image_obj *)image_handle;
>>
>> @@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>> exit_data_size, exit_data);
>>
>> /* Check parameters */
>> - if (image_handle != current_image)
>> - goto out;
>> ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
>> - &info, NULL, NULL,
>> + (void **)&loaded_image_protocol,
>> + NULL, NULL,
>> EFI_OPEN_PROTOCOL_GET_PROTOCOL));
>> - if (ret != EFI_SUCCESS)
>> + if (ret != EFI_SUCCESS) {
>> + ret = EFI_INVALID_PARAMETER;
>> goto out;
>> + }
>> +
>> + /* Unloading of unstarted images */
>> + switch (image_obj->header.type) {
>> + case EFI_OBJECT_TYPE_STARTED_IMAGE:
>> + break;
>> + case EFI_OBJECT_TYPE_LOADED_IMAGE:
>> + efi_delete_image(image_obj, loaded_image_protocol);
>> + ret = EFI_SUCCESS;
>> + goto out;
>> + default:
>> + /* Handle does not refer to loaded image */
>> + ret = EFI_INVALID_PARAMETER;
>> + goto out;
>> + }
>> + /* A started image can only be unloaded it is the last one started. */
>> + if (image_handle != current_image) {
>> + ret = EFI_INVALID_PARAMETER;
>> + goto out;
>> + }
>>
>> /* Exit data is only foreseen in case of failure. */
>> if (exit_status != EFI_SUCCESS) {
>> @@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>> if (ret != EFI_SUCCESS)
>> EFI_PRINT("%s: out of memory\n", __func__);
>> }
>> + if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
>> + exit_status != EFI_SUCCESS)
>> + efi_delete_image(image_obj, loaded_image_protocol);
>
> No change around efi_delete_image() and "goto" above?
>
Do you see a bug?
A diff would help me to understand what you would like to change.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 1/1] efi_loader: unload applications upon Exit()
2019-05-08 0:59 ` Heinrich Schuchardt
@ 2019-05-08 1:08 ` Takahiro Akashi
2019-05-08 5:53 ` Heinrich Schuchardt
0 siblings, 1 reply; 5+ messages in thread
From: Takahiro Akashi @ 2019-05-08 1:08 UTC (permalink / raw)
To: u-boot
On Wed, May 08, 2019 at 02:59:08AM +0200, Heinrich Schuchardt wrote:
> On 5/8/19 1:59 AM, Takahiro Akashi wrote:
> >On Tue, May 07, 2019 at 09:13:24PM +0200, Heinrich Schuchardt wrote:
> >>Implement unloading of images in the Exit() boot services:
> >>
> >>* unload images that are not yet started,
> >>* unload started applications,
> >>* unload drivers returning an error.
> >>
> >>Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>---
> >>v2
> >> Images that are no yet started can be unloaded by calling Exit().
> >> In this case they are not the current image. Move the test for
> >> current down in the code.
> >>
> >> A started driver that called Exit() should still be considered a
> >> started image. Exit cannot be called by another image afterwards,
> >> cf. UEFI SCT 2.6 (2017), 3.5.1 Exit(), 5.1.4.5.8 - 5.1.4.5.10.
> >>---
> >> include/efi_loader.h | 1 +
> >> lib/efi_loader/efi_boottime.c | 36 +++++++++++++++++++++++++------
> >> lib/efi_loader/efi_image_loader.c | 2 ++
> >> 3 files changed, 33 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>index 3b50cd28ef..4e4cffa799 100644
> >>--- a/include/efi_loader.h
> >>+++ b/include/efi_loader.h
> >>@@ -234,6 +234,7 @@ struct efi_loaded_image_obj {
> >> struct jmp_buf_data exit_jmp;
> >> EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
> >> struct efi_system_table *st);
> >>+ u16 image_type;
> >> };
> >>
> >> /**
> >>diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >>index 0385883ded..1ea96dab6c 100644
> >>--- a/lib/efi_loader/efi_boottime.c
> >>+++ b/lib/efi_loader/efi_boottime.c
> >>@@ -13,6 +13,7 @@
> >> #include <linux/libfdt_env.h>
> >> #include <u-boot/crc.h>
> >> #include <bootm.h>
> >>+#include <pe.h>
> >> #include <watchdog.h>
> >>
> >> DECLARE_GLOBAL_DATA_PTR;
> >>@@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >> * image protocol.
> >> */
> >> efi_status_t ret;
> >>- void *info;
> >>+ struct efi_loaded_image *loaded_image_protocol;
> >> struct efi_loaded_image_obj *image_obj =
> >> (struct efi_loaded_image_obj *)image_handle;
> >>
> >>@@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >> exit_data_size, exit_data);
> >>
> >> /* Check parameters */
> >>- if (image_handle != current_image)
> >>- goto out;
> >> ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
> >>- &info, NULL, NULL,
> >>+ (void **)&loaded_image_protocol,
> >>+ NULL, NULL,
> >> EFI_OPEN_PROTOCOL_GET_PROTOCOL));
> >>- if (ret != EFI_SUCCESS)
> >>+ if (ret != EFI_SUCCESS) {
> >>+ ret = EFI_INVALID_PARAMETER;
> >> goto out;
> >>+ }
> >>+
> >>+ /* Unloading of unstarted images */
> >>+ switch (image_obj->header.type) {
> >>+ case EFI_OBJECT_TYPE_STARTED_IMAGE:
> >>+ break;
> >>+ case EFI_OBJECT_TYPE_LOADED_IMAGE:
> >>+ efi_delete_image(image_obj, loaded_image_protocol);
> >>+ ret = EFI_SUCCESS;
> >>+ goto out;
> >>+ default:
> >>+ /* Handle does not refer to loaded image */
> >>+ ret = EFI_INVALID_PARAMETER;
> >>+ goto out;
> >>+ }
> >>+ /* A started image can only be unloaded it is the last one started. */
> >>+ if (image_handle != current_image) {
> >>+ ret = EFI_INVALID_PARAMETER;
> >>+ goto out;
> >>+ }
> >>
> >> /* Exit data is only foreseen in case of failure. */
> >> if (exit_status != EFI_SUCCESS) {
> >>@@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >> if (ret != EFI_SUCCESS)
> >> EFI_PRINT("%s: out of memory\n", __func__);
> >> }
> >>+ if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
> >>+ exit_status != EFI_SUCCESS)
> >>+ efi_delete_image(image_obj, loaded_image_protocol);
> >
> >No change around efi_delete_image() and "goto" above?
> >
>
> Do you see a bug?
>
> A diff would help me to understand what you would like to change.
You said:
>> For me, your code is much unreadable.
>> Moreover, I remember that you have said, in a review of my patch, that
>> we should use "goto" only in error cases.
>
>Good point. So the check must be after handling
>EFI_OBJECT_TYPE_LOADED_IMAGE.
>
>I will revise the patch.
-Takahiro Akashi
> Best regards
>
> Heinrich
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2 1/1] efi_loader: unload applications upon Exit()
2019-05-08 1:08 ` Takahiro Akashi
@ 2019-05-08 5:53 ` Heinrich Schuchardt
0 siblings, 0 replies; 5+ messages in thread
From: Heinrich Schuchardt @ 2019-05-08 5:53 UTC (permalink / raw)
To: u-boot
On 5/8/19 3:08 AM, Takahiro Akashi wrote:
> On Wed, May 08, 2019 at 02:59:08AM +0200, Heinrich Schuchardt wrote:
>> On 5/8/19 1:59 AM, Takahiro Akashi wrote:
>>> On Tue, May 07, 2019 at 09:13:24PM +0200, Heinrich Schuchardt wrote:
>>>> Implement unloading of images in the Exit() boot services:
>>>>
>>>> * unload images that are not yet started,
>>>> * unload started applications,
>>>> * unload drivers returning an error.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>> v2
>>>> Images that are no yet started can be unloaded by calling Exit().
>>>> In this case they are not the current image. Move the test for
>>>> current down in the code.
>>>>
>>>> A started driver that called Exit() should still be considered a
>>>> started image. Exit cannot be called by another image afterwards,
>>>> cf. UEFI SCT 2.6 (2017), 3.5.1 Exit(), 5.1.4.5.8 - 5.1.4.5.10.
>>>> ---
>>>> include/efi_loader.h | 1 +
>>>> lib/efi_loader/efi_boottime.c | 36 +++++++++++++++++++++++++------
>>>> lib/efi_loader/efi_image_loader.c | 2 ++
>>>> 3 files changed, 33 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 3b50cd28ef..4e4cffa799 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -234,6 +234,7 @@ struct efi_loaded_image_obj {
>>>> struct jmp_buf_data exit_jmp;
>>>> EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
>>>> struct efi_system_table *st);
>>>> + u16 image_type;
>>>> };
>>>>
>>>> /**
>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>> index 0385883ded..1ea96dab6c 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -13,6 +13,7 @@
>>>> #include <linux/libfdt_env.h>
>>>> #include <u-boot/crc.h>
>>>> #include <bootm.h>
>>>> +#include <pe.h>
>>>> #include <watchdog.h>
>>>>
>>>> DECLARE_GLOBAL_DATA_PTR;
>>>> @@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>>> * image protocol.
>>>> */
>>>> efi_status_t ret;
>>>> - void *info;
>>>> + struct efi_loaded_image *loaded_image_protocol;
>>>> struct efi_loaded_image_obj *image_obj =
>>>> (struct efi_loaded_image_obj *)image_handle;
>>>>
>>>> @@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>>> exit_data_size, exit_data);
>>>>
>>>> /* Check parameters */
>>>> - if (image_handle != current_image)
>>>> - goto out;
>>>> ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
>>>> - &info, NULL, NULL,
>>>> + (void **)&loaded_image_protocol,
>>>> + NULL, NULL,
>>>> EFI_OPEN_PROTOCOL_GET_PROTOCOL));
>>>> - if (ret != EFI_SUCCESS)
>>>> + if (ret != EFI_SUCCESS) {
>>>> + ret = EFI_INVALID_PARAMETER;
>>>> goto out;
>>>> + }
>>>> +
>>>> + /* Unloading of unstarted images */
>>>> + switch (image_obj->header.type) {
>>>> + case EFI_OBJECT_TYPE_STARTED_IMAGE:
>>>> + break;
>>>> + case EFI_OBJECT_TYPE_LOADED_IMAGE:
>>>> + efi_delete_image(image_obj, loaded_image_protocol);
>>>> + ret = EFI_SUCCESS;
>>>> + goto out;
I think the goto usage here is in accordance with
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions
>>>> + default:
>>>> + /* Handle does not refer to loaded image */
>>>> + ret = EFI_INVALID_PARAMETER;
>>>> + goto out;
>>>> + }
>>>> + /* A started image can only be unloaded it is the last one started. */
>>>> + if (image_handle != current_image) {
>>>> + ret = EFI_INVALID_PARAMETER;
>>>> + goto out;
>>>> + }
These are the lines I moved down.
Regards
Heinrich
>>>>
>>>> /* Exit data is only foreseen in case of failure. */
>>>> if (exit_status != EFI_SUCCESS) {
>>>> @@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>>> if (ret != EFI_SUCCESS)
>>>> EFI_PRINT("%s: out of memory\n", __func__);
>>>> }
>>>> + if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
>>>> + exit_status != EFI_SUCCESS)
>>>> + efi_delete_image(image_obj, loaded_image_protocol);
>>>
>>> No change around efi_delete_image() and "goto" above?
>>>
>>
>> Do you see a bug?
>>
>> A diff would help me to understand what you would like to change.
>
> You said:
>>> For me, your code is much unreadable.
>>> Moreover, I remember that you have said, in a review of my patch, that
>>> we should use "goto" only in error cases.
>>
>> Good point. So the check must be after handling
>> EFI_OBJECT_TYPE_LOADED_IMAGE.
>>
>> I will revise the patch.
>
> -Takahiro Akashi
>
>> Best regards
>>
>> Heinrich
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-08 5:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-07 19:13 [U-Boot] [PATCH v2 1/1] efi_loader: unload applications upon Exit() Heinrich Schuchardt
2019-05-07 23:59 ` Takahiro Akashi
2019-05-08 0:59 ` Heinrich Schuchardt
2019-05-08 1:08 ` Takahiro Akashi
2019-05-08 5:53 ` Heinrich Schuchardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox