[U-Boot] [PATCH 0/4] efi_loader implent unloading of images
The patch series implements the UnloadImage() boot service and the unloading of images in Exit().
Heinrich Schuchardt (4): efi_loader: mark started images efi_loader: move efi_unload_image() down in source efi_loader: implement UnloadImage() efi_loader: unload applications upon Exit()
include/efi_api.h | 2 +- include/efi_loader.h | 14 ++++ lib/efi_loader/efi_boottime.c | 129 +++++++++++++++++++++++------- lib/efi_loader/efi_image_loader.c | 2 + 4 files changed, 118 insertions(+), 29 deletions(-)
-- 2.20.1
In UnloadImage() we need to know if an image is already started.
Add a field to the handle structure identifying loaded and started images.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_boottime.c | 2 ++ 2 files changed, 15 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3fd9901d66..c2a449e5b6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -182,6 +182,18 @@ struct efi_handler { struct list_head open_infos; };
+/** + * enum efi_object_type - type of EFI object + * + * In UnloadImage we must be able to identify if the handle relates to a + * started image. + */ +enum efi_object_type { + EFI_OBJECT_TYPE_UNDEFINED = 0, + EFI_OBJECT_TYPE_LOADED_IMAGE, + EFI_OBJECT_TYPE_STARTED_IMAGE, +}; + /** * struct efi_object - dereferenced EFI handle * @@ -204,6 +216,7 @@ struct efi_object { struct list_head link; /* The list of protocols */ struct list_head protocols; + enum efi_object_type type; };
/** diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3ed08e7c37..dc444fccf6 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, free(info); return EFI_OUT_OF_RESOURCES; } + obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* Add internal object to object list */ efi_add_handle(&obj->header); @@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, }
current_image = image_handle; + image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE; EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); ret = EFI_CALL(image_obj->entry(image_handle, &systab));
-- 2.20.1
On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
In UnloadImage() we need to know if an image is already started.
Add a field to the handle structure identifying loaded and started images.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_boottime.c | 2 ++ 2 files changed, 15 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3fd9901d66..c2a449e5b6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -182,6 +182,18 @@ struct efi_handler { struct list_head open_infos; };
+/**
- enum efi_object_type - type of EFI object
- In UnloadImage we must be able to identify if the handle relates to a
- started image.
- */
+enum efi_object_type {
- EFI_OBJECT_TYPE_UNDEFINED = 0,
- EFI_OBJECT_TYPE_LOADED_IMAGE,
- EFI_OBJECT_TYPE_STARTED_IMAGE,
+};
It sounds *status*, not *type*. In a separate patch, you added U_BOOT_FIRMWARE. We should distinguish status and type for future enhancement and to avoid any confusion.
Thanks, -Takahiro Akashi
/**
- struct efi_object - dereferenced EFI handle
@@ -204,6 +216,7 @@ struct efi_object { struct list_head link; /* The list of protocols */ struct list_head protocols;
- enum efi_object_type type;
};
/** diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3ed08e7c37..dc444fccf6 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, free(info); return EFI_OUT_OF_RESOURCES; }
obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* Add internal object to object list */ efi_add_handle(&obj->header);
@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, }
current_image = image_handle;
- image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE; EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); ret = EFI_CALL(image_obj->entry(image_handle, &systab));
-- 2.20.1
On 5/7/19 5:02 AM, Takahiro Akashi wrote:
On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
In UnloadImage() we need to know if an image is already started.
Add a field to the handle structure identifying loaded and started images.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_boottime.c | 2 ++ 2 files changed, 15 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3fd9901d66..c2a449e5b6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -182,6 +182,18 @@ struct efi_handler { struct list_head open_infos; };
+/**
- enum efi_object_type - type of EFI object
- In UnloadImage we must be able to identify if the handle relates to a
- started image.
- */
+enum efi_object_type {
- EFI_OBJECT_TYPE_UNDEFINED = 0,
- EFI_OBJECT_TYPE_LOADED_IMAGE,
- EFI_OBJECT_TYPE_STARTED_IMAGE,
+};
It sounds *status*, not *type*. In a separate patch, you added U_BOOT_FIRMWARE. We should distinguish status and type for future enhancement and to avoid any confusion.
Having both information in the same field makes the evaluation easier.
Currently I see not necessity for more handle types to be differentiated. Let's change it when the need arises.
Best regards
Heinrich
Thanks, -Takahiro Akashi
/**
- struct efi_object - dereferenced EFI handle
@@ -204,6 +216,7 @@ struct efi_object { struct list_head link; /* The list of protocols */ struct list_head protocols;
enum efi_object_type type; };
/**
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3ed08e7c37..dc444fccf6 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, free(info); return EFI_OUT_OF_RESOURCES; }
obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* Add internal object to object list */ efi_add_handle(&obj->header);
@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, }
current_image = image_handle;
- image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE; EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); ret = EFI_CALL(image_obj->entry(image_handle, &systab));
-- 2.20.1
On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 5:02 AM, Takahiro Akashi wrote:
On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
In UnloadImage() we need to know if an image is already started.
Add a field to the handle structure identifying loaded and started images.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_boottime.c | 2 ++ 2 files changed, 15 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3fd9901d66..c2a449e5b6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -182,6 +182,18 @@ struct efi_handler { struct list_head open_infos; };
+/**
- enum efi_object_type - type of EFI object
- In UnloadImage we must be able to identify if the handle relates to a
- started image.
- */
+enum efi_object_type {
- EFI_OBJECT_TYPE_UNDEFINED = 0,
- EFI_OBJECT_TYPE_LOADED_IMAGE,
- EFI_OBJECT_TYPE_STARTED_IMAGE,
+};
It sounds *status*, not *type*. In a separate patch, you added U_BOOT_FIRMWARE. We should distinguish status and type for future enhancement and to avoid any confusion.
Having both information in the same field makes the evaluation easier.
Currently I see not necessity for more handle types to be differentiated. Let's change it when the need arises.
I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE for root node. Then we can rename efi_object_type to efi_image_status.
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
/**
- struct efi_object - dereferenced EFI handle
@@ -204,6 +216,7 @@ struct efi_object { struct list_head link; /* The list of protocols */ struct list_head protocols;
- enum efi_object_type type;
};
/** diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3ed08e7c37..dc444fccf6 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, free(info); return EFI_OUT_OF_RESOURCES; }
obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* Add internal object to object list */ efi_add_handle(&obj->header);
@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, }
current_image = image_handle;
- image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE; EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); ret = EFI_CALL(image_obj->entry(image_handle, &systab));
-- 2.20.1
On 5/7/19 7:44 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 5:02 AM, Takahiro Akashi wrote:
On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
In UnloadImage() we need to know if an image is already started.
Add a field to the handle structure identifying loaded and started images.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_boottime.c | 2 ++ 2 files changed, 15 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3fd9901d66..c2a449e5b6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -182,6 +182,18 @@ struct efi_handler { struct list_head open_infos; };
+/**
- enum efi_object_type - type of EFI object
- In UnloadImage we must be able to identify if the handle relates to a
- started image.
- */
+enum efi_object_type {
- EFI_OBJECT_TYPE_UNDEFINED = 0,
- EFI_OBJECT_TYPE_LOADED_IMAGE,
- EFI_OBJECT_TYPE_STARTED_IMAGE,
+};
It sounds *status*, not *type*. In a separate patch, you added U_BOOT_FIRMWARE. We should distinguish status and type for future enhancement and to avoid any confusion.
Having both information in the same field makes the evaluation easier.
Currently I see not necessity for more handle types to be differentiated. Let's change it when the need arises.
I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE for root node. Then we can rename efi_object_type to efi_image_status.
This would allow calling UnloadImage() for the root node.
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
/**
- struct efi_object - dereferenced EFI handle
@@ -204,6 +216,7 @@ struct efi_object { struct list_head link; /* The list of protocols */ struct list_head protocols;
enum efi_object_type type; };
/**
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3ed08e7c37..dc444fccf6 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, free(info); return EFI_OUT_OF_RESOURCES; }
obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* Add internal object to object list */ efi_add_handle(&obj->header);
@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, }
current_image = image_handle;
- image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE; EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); ret = EFI_CALL(image_obj->entry(image_handle, &systab));
-- 2.20.1
On Tue, May 07, 2019 at 07:53:41AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 7:44 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 5:02 AM, Takahiro Akashi wrote:
On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
In UnloadImage() we need to know if an image is already started.
Add a field to the handle structure identifying loaded and started images.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_boottime.c | 2 ++ 2 files changed, 15 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3fd9901d66..c2a449e5b6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -182,6 +182,18 @@ struct efi_handler { struct list_head open_infos; };
+/**
- enum efi_object_type - type of EFI object
- In UnloadImage we must be able to identify if the handle relates to a
- started image.
- */
+enum efi_object_type {
- EFI_OBJECT_TYPE_UNDEFINED = 0,
- EFI_OBJECT_TYPE_LOADED_IMAGE,
- EFI_OBJECT_TYPE_STARTED_IMAGE,
+};
It sounds *status*, not *type*. In a separate patch, you added U_BOOT_FIRMWARE. We should distinguish status and type for future enhancement and to avoid any confusion.
Having both information in the same field makes the evaluation easier.
Currently I see not necessity for more handle types to be differentiated. Let's change it when the need arises.
I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE for root node. Then we can rename efi_object_type to efi_image_status.
This would allow calling UnloadImage() for the root node.
Who knows the address of root node? For safe guard, you can add if (handle == root_node) return EFI_INVALID_PARAMETER; at the beginning of unload_image.
-Takahiro Akashi
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
/**
- struct efi_object - dereferenced EFI handle
@@ -204,6 +216,7 @@ struct efi_object { struct list_head link; /* The list of protocols */ struct list_head protocols;
- enum efi_object_type type;
};
/** diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3ed08e7c37..dc444fccf6 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, free(info); return EFI_OUT_OF_RESOURCES; }
obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* Add internal object to object list */ efi_add_handle(&obj->header);
@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, }
current_image = image_handle;
- image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE; EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); ret = EFI_CALL(image_obj->entry(image_handle, &systab));
-- 2.20.1
On 5/7/19 7:58 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 07:53:41AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 7:44 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 5:02 AM, Takahiro Akashi wrote:
On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
In UnloadImage() we need to know if an image is already started.
Add a field to the handle structure identifying loaded and started images.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_boottime.c | 2 ++ 2 files changed, 15 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3fd9901d66..c2a449e5b6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -182,6 +182,18 @@ struct efi_handler { struct list_head open_infos; };
+/**
- enum efi_object_type - type of EFI object
- In UnloadImage we must be able to identify if the handle relates to a
- started image.
- */
+enum efi_object_type {
- EFI_OBJECT_TYPE_UNDEFINED = 0,
- EFI_OBJECT_TYPE_LOADED_IMAGE,
- EFI_OBJECT_TYPE_STARTED_IMAGE,
+};
It sounds *status*, not *type*. In a separate patch, you added U_BOOT_FIRMWARE. We should distinguish status and type for future enhancement and to avoid any confusion.
Having both information in the same field makes the evaluation easier.
Currently I see not necessity for more handle types to be differentiated. Let's change it when the need arises.
I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE for root node. Then we can rename efi_object_type to efi_image_status.
This would allow calling UnloadImage() for the root node.
Who knows the address of root node?
Just enumerated handles with a loaded image protocol.
For safe guard, you can add if (handle == root_node) return EFI_INVALID_PARAMETER; at the beginning of unload_image.
This would not decrease complexity.
Regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
/**
- struct efi_object - dereferenced EFI handle
@@ -204,6 +216,7 @@ struct efi_object { struct list_head link; /* The list of protocols */ struct list_head protocols;
enum efi_object_type type; };
/**
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3ed08e7c37..dc444fccf6 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, free(info); return EFI_OUT_OF_RESOURCES; }
obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* Add internal object to object list */ efi_add_handle(&obj->header);
@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, }
current_image = image_handle;
- image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE; EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); ret = EFI_CALL(image_obj->entry(image_handle, &systab));
-- 2.20.1
On Tue, May 07, 2019 at 08:05:48AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 7:58 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 07:53:41AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 7:44 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 5:02 AM, Takahiro Akashi wrote:
On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote: >In UnloadImage() we need to know if an image is already started. > >Add a field to the handle structure identifying loaded and started images. > >Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de >--- > include/efi_loader.h | 13 +++++++++++++ > lib/efi_loader/efi_boottime.c | 2 ++ > 2 files changed, 15 insertions(+) > >diff --git a/include/efi_loader.h b/include/efi_loader.h >index 3fd9901d66..c2a449e5b6 100644 >--- a/include/efi_loader.h >+++ b/include/efi_loader.h >@@ -182,6 +182,18 @@ struct efi_handler { > struct list_head open_infos; > }; > >+/** >+ * enum efi_object_type - type of EFI object >+ * >+ * In UnloadImage we must be able to identify if the handle relates to a >+ * started image. >+ */ >+enum efi_object_type { >+ EFI_OBJECT_TYPE_UNDEFINED = 0, >+ EFI_OBJECT_TYPE_LOADED_IMAGE, >+ EFI_OBJECT_TYPE_STARTED_IMAGE, >+};
It sounds *status*, not *type*. In a separate patch, you added U_BOOT_FIRMWARE. We should distinguish status and type for future enhancement and to avoid any confusion.
Having both information in the same field makes the evaluation easier.
Currently I see not necessity for more handle types to be differentiated. Let's change it when the need arises.
I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE for root node. Then we can rename efi_object_type to efi_image_status.
This would allow calling UnloadImage() for the root node.
Who knows the address of root node?
Just enumerated handles with a loaded image protocol.
Remove it when enumerating handles if necessary.
For safe guard, you can add if (handle == root_node) return EFI_INVALID_PARAMETER; at the beginning of unload_image.
This would not decrease complexity.
I believe that it's much better and more understandable than confusing use of status and type.
-Takahiro Akashi
Regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
> /** > * struct efi_object - dereferenced EFI handle > * >@@ -204,6 +216,7 @@ struct efi_object { > struct list_head link; > /* The list of protocols */ > struct list_head protocols; >+ enum efi_object_type type; > }; > > /** >diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >index 3ed08e7c37..dc444fccf6 100644 >--- a/lib/efi_loader/efi_boottime.c >+++ b/lib/efi_loader/efi_boottime.c >@@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, > free(info); > return EFI_OUT_OF_RESOURCES; > } >+ obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE; > > /* Add internal object to object list */ > efi_add_handle(&obj->header); >@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, > } > > current_image = image_handle; >+ image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE; > EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); > ret = EFI_CALL(image_obj->entry(image_handle, &systab)); > >-- >2.20.1 >
On 5/7/19 8:12 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 08:05:48AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 7:58 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 07:53:41AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 7:44 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 5:02 AM, Takahiro Akashi wrote: > On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote: >> In UnloadImage() we need to know if an image is already started. >> >> Add a field to the handle structure identifying loaded and started images. >> >> Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de >> --- >> include/efi_loader.h | 13 +++++++++++++ >> lib/efi_loader/efi_boottime.c | 2 ++ >> 2 files changed, 15 insertions(+) >> >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 3fd9901d66..c2a449e5b6 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -182,6 +182,18 @@ struct efi_handler { >> struct list_head open_infos; >> }; >> >> +/** >> + * enum efi_object_type - type of EFI object >> + * >> + * In UnloadImage we must be able to identify if the handle relates to a >> + * started image. >> + */ >> +enum efi_object_type { >> + EFI_OBJECT_TYPE_UNDEFINED = 0, >> + EFI_OBJECT_TYPE_LOADED_IMAGE, >> + EFI_OBJECT_TYPE_STARTED_IMAGE, >> +}; > > It sounds *status*, not *type*. > In a separate patch, you added U_BOOT_FIRMWARE. > We should distinguish status and type for future enhancement and > to avoid any confusion.
Having both information in the same field makes the evaluation easier.
Currently I see not necessity for more handle types to be differentiated. Let's change it when the need arises.
I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE for root node. Then we can rename efi_object_type to efi_image_status.
This would allow calling UnloadImage() for the root node.
Who knows the address of root node?
Just enumerated handles with a loaded image protocol.
Remove it when enumerating handles if necessary.
No, we have to return the handle when LocateHandle() is called for the loaded image protocol.
Regards
Heinrich
For safe guard, you can add if (handle == root_node) return EFI_INVALID_PARAMETER; at the beginning of unload_image.
This would not decrease complexity.
I believe that it's much better and more understandable than confusing use of status and type.
-Takahiro Akashi
Regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
> > Thanks, > -Takahiro Akashi > > >> /** >> * struct efi_object - dereferenced EFI handle >> * >> @@ -204,6 +216,7 @@ struct efi_object { >> struct list_head link; >> /* The list of protocols */ >> struct list_head protocols; >> + enum efi_object_type type; >> }; >> >> /** >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >> index 3ed08e7c37..dc444fccf6 100644 >> --- a/lib/efi_loader/efi_boottime.c >> +++ b/lib/efi_loader/efi_boottime.c >> @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, >> free(info); >> return EFI_OUT_OF_RESOURCES; >> } >> + obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE; >> >> /* Add internal object to object list */ >> efi_add_handle(&obj->header); >> @@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, >> } >> >> current_image = image_handle; >> + image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE; >> EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); >> ret = EFI_CALL(image_obj->entry(image_handle, &systab)); >> >> -- >> 2.20.1 >> >
Move efi_unload_image() down in source to avoid forward declaration in following page.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- lib/efi_loader/efi_boottime.c | 46 +++++++++++++++++------------------ 1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index dc444fccf6..2992af269a 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1744,29 +1744,6 @@ error: return EFI_EXIT(ret); }
-/** - * efi_unload_image() - unload an EFI image - * @image_handle: handle of the image to be unloaded - * - * This function implements the UnloadImage service. - * - * See the Unified Extensible Firmware Interface (UEFI) specification for - * details. - * - * Return: status code - */ -efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle) -{ - struct efi_object *efiobj; - - EFI_ENTRY("%p", image_handle); - efiobj = efi_search_obj(image_handle); - if (efiobj) - list_del(&efiobj->link); - - return EFI_EXIT(EFI_SUCCESS); -} - /** * efi_exit_caches() - fix up caches for EFI payloads if necessary */ @@ -2692,6 +2669,29 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, return EFI_CALL(systab.boottime->exit(image_handle, ret, 0, NULL)); }
+/** + * efi_unload_image() - unload an EFI image + * @image_handle: handle of the image to be unloaded + * + * This function implements the UnloadImage service. + * + * See the Unified Extensible Firmware Interface (UEFI) specification for + * details. + * + * Return: status code + */ +efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle) +{ + struct efi_object *efiobj; + + EFI_ENTRY("%p", image_handle); + efiobj = efi_search_obj(image_handle); + if (efiobj) + list_del(&efiobj->link); + + return EFI_EXIT(EFI_SUCCESS); +} + /** * efi_update_exit_data() - fill exit data parameters of StartImage() * -- 2.20.1
Implement the UnloadImage() boot service
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- include/efi_api.h | 2 +- lib/efi_loader/efi_boottime.c | 55 ++++++++++++++++++++++++++++++++--- 2 files changed, 52 insertions(+), 5 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 4ebb4a5f59..54c6232079 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -348,7 +348,7 @@ struct efi_loaded_image { aligned_u64 image_size; unsigned int image_code_type; unsigned int image_data_type; - unsigned long unload; + efi_status_t (EFIAPI *unload)(efi_handle_t image_handle); };
#define EFI_DEVICE_PATH_PROTOCOL_GUID \ diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 2992af269a..bbcd66caa6 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -2669,6 +2669,20 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, return EFI_CALL(systab.boottime->exit(image_handle, ret, 0, NULL)); }
+/** + * efi_delete_image() - delete loaded image from memory) + * + * @image_obj: handle of the loaded image + * @loaded_image_protocol: loaded image protocol + */ +static void efi_delete_image(struct efi_loaded_image_obj *image_obj, + struct efi_loaded_image *loaded_image_protocol) +{ + efi_free_pages((uintptr_t)loaded_image_protocol->image_base, + efi_size_in_pages(loaded_image_protocol->image_size)); + efi_delete_handle(&image_obj->header); +} + /** * efi_unload_image() - unload an EFI image * @image_handle: handle of the image to be unloaded @@ -2682,14 +2696,47 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, */ efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle) { + efi_status_t ret = EFI_SUCCESS; struct efi_object *efiobj; + struct efi_loaded_image *loaded_image_protocol;
EFI_ENTRY("%p", image_handle); - efiobj = efi_search_obj(image_handle); - if (efiobj) - list_del(&efiobj->link);
- return EFI_EXIT(EFI_SUCCESS); + efiobj = efi_search_obj(image_handle); + if (!efiobj) { + ret = EFI_INVALID_PARAMETER; + goto out; + } + /* Find the loaded image protocol */ + ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image, + (void **)&loaded_image_protocol, + NULL, NULL, + EFI_OPEN_PROTOCOL_GET_PROTOCOL)); + if (ret != EFI_SUCCESS) { + ret = EFI_INVALID_PARAMETER; + goto out; + } + switch (efiobj->type) { + case EFI_OBJECT_TYPE_STARTED_IMAGE: + /* Call the unload function */ + if (!loaded_image_protocol->unload) { + ret = EFI_UNSUPPORTED; + goto out; + } + ret = EFI_CALL(loaded_image_protocol->unload(image_handle)); + if (ret != EFI_SUCCESS) + goto out; + break; + case EFI_OBJECT_TYPE_LOADED_IMAGE: + break; + default: + ret = EFI_INVALID_PARAMETER; + goto out; + } + efi_delete_image((struct efi_loaded_image_obj *)efiobj, + loaded_image_protocol); +out: + return EFI_EXIT(ret); }
/** -- 2.20.1
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 --- include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 34 ++++++++++++++++++++++++++----- lib/efi_loader/efi_image_loader.c | 2 ++ 3 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index c2a449e5b6..d73c89ac26 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -237,6 +237,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 bbcd66caa6..51e0bb2fd5 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) + if (image_handle != current_image) { + ret = EFI_INVALID_PARAMETER; 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; + } + image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* 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 Sat, May 04, 2019 at 10:36:36AM +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
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 34 ++++++++++++++++++++++++++----- lib/efi_loader/efi_image_loader.c | 2 ++ 3 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index c2a449e5b6..d73c89ac26 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -237,6 +237,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 bbcd66caa6..51e0bb2fd5 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)
- if (image_handle != current_image) {
Does this check make sense even for a driver?
ret = EFI_INVALID_PARAMETER;- } 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) {
Is this check necessary even when 'header.type' is introduced?
ret = EFI_INVALID_PARAMETER;- }
- /* Unloading of unstarted images */
'Unload' sounds odd when we call efi_delete_image(), doesn't it?
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;}
image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* 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);
Why not merge those two efi_delete_image()?
-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/7/19 6:39 AM, Takahiro Akashi wrote:
On Sat, May 04, 2019 at 10:36:36AM +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
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 34 ++++++++++++++++++++++++++----- lib/efi_loader/efi_image_loader.c | 2 ++ 3 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index c2a449e5b6..d73c89ac26 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -237,6 +237,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 bbcd66caa6..51e0bb2fd5 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)
- if (image_handle != current_image) {
Does this check make sense even for a driver?
The check is prescribed in the UEFI specification. See the heading "Status Codes Returned".
Multiple binaries may be in status started. If a child image (which may be a driver) calls Exit() with the parent image handle this might cause fancy problems.
The lifetime of a driver is LoadImage(), StartImage(), Exit(), UnloadImage(). Typically between StartImage() and Exit() it will install its protocols and between Exit() and UnloadImage() wait for other binaries to call the protocols.
ret = EFI_INVALID_PARAMETER;- } 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) {
Is this check necessary even when 'header.type' is introduced?
I am passing the loaded image protocol to efi_delete_handle(). In principal a binary might have uninstalled its own loaded image protocol so this call may fail.
ret = EFI_INVALID_PARAMETER;- }
- /* Unloading of unstarted images */
'Unload' sounds odd when we call efi_delete_image(), doesn't it?
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;}
image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* 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);Why not merge those two efi_delete_image()?
I went for readable code. The if statement catching all cases was unreadable to me.
Best regards
Heinrich
-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 Tue, May 07, 2019 at 07:50:48AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 6:39 AM, Takahiro Akashi wrote:
On Sat, May 04, 2019 at 10:36:36AM +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
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 34 ++++++++++++++++++++++++++----- lib/efi_loader/efi_image_loader.c | 2 ++ 3 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index c2a449e5b6..d73c89ac26 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -237,6 +237,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 bbcd66caa6..51e0bb2fd5 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)
- if (image_handle != current_image) {
Does this check make sense even for a driver?
The check is prescribed in the UEFI specification. See the heading "Status Codes Returned".
Multiple binaries may be in status started. If a child image (which may be a driver) calls Exit() with the parent image handle this might cause fancy problems.
The lifetime of a driver is LoadImage(), StartImage(), Exit(), UnloadImage(). Typically between StartImage() and Exit() it will install its protocols and between Exit() and UnloadImage() wait for other binaries to call the protocols.
If this is true, that will be fine for a driver. But what about 'not started' application? In "Status Codes Returned" of Exit(), it says EFI_SUCCESS The image specified by ImageHandle was unloaded. This condition only occurs for images that have been loaded with LoadImage() but have not been started with StartImage(). In this case, the caller of Exit() is not an application.
ret = EFI_INVALID_PARAMETER;- } 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) {
Is this check necessary even when 'header.type' is introduced?
I am passing the loaded image protocol to efi_delete_handle().
I don't think that it can be a critical reason.
In principal a binary might have uninstalled its own loaded image protocol so this call may fail.
If we admit this possibility, there will be bunch of fragile code elsewhere.
ret = EFI_INVALID_PARAMETER;- }
- /* Unloading of unstarted images */
'Unload' sounds odd when we call efi_delete_image(), doesn't it?
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;}
image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* 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);Why not merge those two efi_delete_image()?
I went for readable code. The if statement catching all cases was unreadable to me.
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.
-Takahiro Akashi
Best regards
Heinrich
-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/7/19 8:37 AM, Takahiro Akashi wrote:
On Tue, May 07, 2019 at 07:50:48AM +0200, Heinrich Schuchardt wrote:
On 5/7/19 6:39 AM, Takahiro Akashi wrote:
On Sat, May 04, 2019 at 10:36:36AM +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
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 34 ++++++++++++++++++++++++++----- lib/efi_loader/efi_image_loader.c | 2 ++ 3 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index c2a449e5b6..d73c89ac26 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -237,6 +237,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 bbcd66caa6..51e0bb2fd5 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)
- if (image_handle != current_image) {
Does this check make sense even for a driver?
The check is prescribed in the UEFI specification. See the heading "Status Codes Returned".
Multiple binaries may be in status started. If a child image (which may be a driver) calls Exit() with the parent image handle this might cause fancy problems.
The lifetime of a driver is LoadImage(), StartImage(), Exit(), UnloadImage(). Typically between StartImage() and Exit() it will install its protocols and between Exit() and UnloadImage() wait for other binaries to call the protocols.
If this is true, that will be fine for a driver. But what about 'not started' application? In "Status Codes Returned" of Exit(), it says EFI_SUCCESS The image specified by ImageHandle was unloaded. This condition only occurs for images that have been loaded with LoadImage() but have not been started with StartImage(). In this case, the caller of Exit() is not an application.
ret = EFI_INVALID_PARAMETER;- } 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) {
Is this check necessary even when 'header.type' is introduced?
I am passing the loaded image protocol to efi_delete_handle().
I don't think that it can be a critical reason.
In principal a binary might have uninstalled its own loaded image protocol so this call may fail.
If we admit this possibility, there will be bunch of fragile code elsewhere.
ret = EFI_INVALID_PARAMETER;- }
- /* Unloading of unstarted images */
'Unload' sounds odd when we call efi_delete_image(), doesn't it?
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;}
image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
/* 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);Why not merge those two efi_delete_image()?
I went for readable code. The if statement catching all cases was unreadable to me.
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.
Thanks for reviewing.
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
-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
participants (2)
-
Heinrich Schuchardt -
Takahiro Akashi