[U-Boot] [PATCH v2 1/1] efi_loader: unload applications upon Exit()
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
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;&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;@@ -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;-- 2.20.1
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;&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
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;&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
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;&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;- }
- /* 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-...
- 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
participants (2)
-
Heinrich Schuchardt -
Takahiro Akashi