[U-Boot] [PATCH v2 1/1] efi_loader: LoadImage() parameter checks
If the file path is NULL, return EFI_INVALID_PARAMETER. If the file path is invalid, return EFI_NOT_FOUND. If the size of the source buffer is 0, return EFI_LOAD_ERROR. If the parent image handle does not refer to a loaded image return EFI_INVALID_PARAMETER.
The change is required to reach UEFI SCT conformance.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 efi_load_image_from_path() must initalize *buffer even in case of an error --- include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 17 ++++++++---- lib/efi_loader/efi_root_node.c | 48 ++++++++++++++++++---------------- 3 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index d3a1d4c465..07ef14ba1c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -187,6 +187,7 @@ struct efi_handler { */ enum efi_object_type { EFI_OBJECT_TYPE_UNDEFINED = 0, + EFI_OBJECT_TYPE_U_BOOT_FIRMWARE, EFI_OBJECT_TYPE_LOADED_IMAGE, EFI_OBJECT_TYPE_STARTED_IMAGE, }; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 78a4063949..f75e6c0169 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1680,10 +1680,13 @@ efi_status_t efi_load_image_from_path(struct efi_device_path *file_path, *buffer = NULL; *size = 0;
+ if (!file_path) + return EFI_INVALID_PARAMETER; + /* Open file */ f = efi_file_from_path(file_path); if (!f) - return EFI_DEVICE_ERROR; + return EFI_NOT_FOUND;
/* Get file size */ bs = 0; @@ -1760,13 +1763,13 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, EFI_ENTRY("%d, %p, %pD, %p, %zd, %p", boot_policy, parent_image, file_path, source_buffer, source_size, image_handle);
- if (!image_handle || !parent_image) { + if (!image_handle || !efi_search_obj(parent_image)) { ret = EFI_INVALID_PARAMETER; goto error; } - - if (!source_buffer && !file_path) { - ret = EFI_NOT_FOUND; + /* The parent image handle must refer to a loaded image */ + if (!parent_image->type) { + ret = EFI_INVALID_PARAMETER; goto error; }
@@ -1776,6 +1779,10 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, if (ret != EFI_SUCCESS) goto error; } else { + if (!source_size) { + ret = EFI_LOAD_ERROR; + goto error; + } dest_buffer = source_buffer; } /* split file_path which contains both the device and file parts */ diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c index e0fcbb85a4..38514e0820 100644 --- a/lib/efi_loader/efi_root_node.c +++ b/lib/efi_loader/efi_root_node.c @@ -28,6 +28,7 @@ struct efi_root_dp { */ efi_status_t efi_root_node_register(void) { + efi_status_t ret; struct efi_root_dp *dp;
/* Create device path protocol */ @@ -47,28 +48,31 @@ efi_status_t efi_root_node_register(void) dp->end.length = sizeof(struct efi_device_path);
/* Create root node and install protocols */ - return EFI_CALL(efi_install_multiple_protocol_interfaces(&efi_root, - /* Device path protocol */ - &efi_guid_device_path, dp, - /* Device path to text protocol */ - &efi_guid_device_path_to_text_protocol, - (void *)&efi_device_path_to_text, - /* Device path utilities protocol */ - &efi_guid_device_path_utilities_protocol, - (void *)&efi_device_path_utilities, - /* Unicode collation protocol */ - &efi_guid_unicode_collation_protocol, - (void *)&efi_unicode_collation_protocol, + ret = EFI_CALL(efi_install_multiple_protocol_interfaces + (&efi_root, + /* Device path protocol */ + &efi_guid_device_path, dp, + /* Device path to text protocol */ + &efi_guid_device_path_to_text_protocol, + (void *)&efi_device_path_to_text, + /* Device path utilities protocol */ + &efi_guid_device_path_utilities_protocol, + (void *)&efi_device_path_utilities, + /* Unicode collation protocol */ + &efi_guid_unicode_collation_protocol, + (void *)&efi_unicode_collation_protocol, #if CONFIG_IS_ENABLED(EFI_LOADER_HII) - /* HII string protocol */ - &efi_guid_hii_string_protocol, - (void *)&efi_hii_string, - /* HII database protocol */ - &efi_guid_hii_database_protocol, - (void *)&efi_hii_database, - /* HII configuration routing protocol */ - &efi_guid_hii_config_routing_protocol, - (void *)&efi_hii_config_routing, + /* HII string protocol */ + &efi_guid_hii_string_protocol, + (void *)&efi_hii_string, + /* HII database protocol */ + &efi_guid_hii_database_protocol, + (void *)&efi_hii_database, + /* HII configuration routing protocol */ + &efi_guid_hii_config_routing_protocol, + (void *)&efi_hii_config_routing, #endif - NULL)); + NULL)); + efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE; + return ret; } -- 2.20.1
On 06.05.19 07:42, Heinrich Schuchardt wrote:
If the file path is NULL, return EFI_INVALID_PARAMETER. If the file path is invalid, return EFI_NOT_FOUND.
These 2 are a documented inEFI_LOAD_FILE_PROTOCOL.LoadFile(). It might be a good idea to indicate that for later reference.
If the size of the source buffer is 0, return EFI_LOAD_ERROR.
The spec says this should be EFI_INVALID_PARAMETER, no? Is this a spec or an SCT bug?
If the parent image handle does not refer to a loaded image return EFI_INVALID_PARAMETER.
I agree that this is a good change, but where is the spec reference? I couldn't find anything.
The change is required to reach UEFI SCT conformance.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
IMHO this really should be a patch set with multiple individual changes, each changing one piece of spec conformance at a time.
That way if we happen to break anything along the way, bisecting would point us to exactly the right point where things fell apart.
v2 efi_load_image_from_path() must initalize *buffer even in case of an error
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 17 ++++++++---- lib/efi_loader/efi_root_node.c | 48 ++++++++++++++++++---------------- 3 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index d3a1d4c465..07ef14ba1c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -187,6 +187,7 @@ struct efi_handler { */ enum efi_object_type { EFI_OBJECT_TYPE_UNDEFINED = 0,
- EFI_OBJECT_TYPE_U_BOOT_FIRMWARE,
This looks like an unrelated change?
EFI_OBJECT_TYPE_LOADED_IMAGE, EFI_OBJECT_TYPE_STARTED_IMAGE, }; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 78a4063949..f75e6c0169 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1680,10 +1680,13 @@ efi_status_t efi_load_image_from_path(struct efi_device_path *file_path, *buffer = NULL; *size = 0;
- if (!file_path)
return EFI_INVALID_PARAMETER;- /* Open file */ f = efi_file_from_path(file_path); if (!f)
return EFI_DEVICE_ERROR;
return EFI_NOT_FOUND;/* Get file size */ bs = 0;
@@ -1760,13 +1763,13 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, EFI_ENTRY("%d, %p, %pD, %p, %zd, %p", boot_policy, parent_image, file_path, source_buffer, source_size, image_handle);
- if (!image_handle || !parent_image) {
- if (!image_handle || !efi_search_obj(parent_image)) { ret = EFI_INVALID_PARAMETER; goto error; }
- if (!source_buffer && !file_path) {
ret = EFI_NOT_FOUND;
- /* The parent image handle must refer to a loaded image */
- if (!parent_image->type) {
ret = EFI_INVALID_PARAMETER;@@ -1776,6 +1779,10 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, if (ret != EFI_SUCCESS) goto error; } else {
if (!source_size) {ret = EFI_LOAD_ERROR;goto error;}diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c index e0fcbb85a4..38514e0820 100644 --- a/lib/efi_loader/efi_root_node.c +++ b/lib/efi_loader/efi_root_node.c @@ -28,6 +28,7 @@ struct efi_root_dp { */ efi_status_t efi_root_node_register(void) {
efi_status_t ret; struct efi_root_dp *dp;
/* Create device path protocol */
@@ -47,28 +48,31 @@ efi_status_t efi_root_node_register(void) dp->end.length = sizeof(struct efi_device_path);
/* Create root node and install protocols */
- return EFI_CALL(efi_install_multiple_protocol_interfaces(&efi_root,
/* Device path protocol */&efi_guid_device_path, dp,/* Device path to text protocol */&efi_guid_device_path_to_text_protocol,(void *)&efi_device_path_to_text,/* Device path utilities protocol */&efi_guid_device_path_utilities_protocol,(void *)&efi_device_path_utilities,/* Unicode collation protocol */&efi_guid_unicode_collation_protocol,(void *)&efi_unicode_collation_protocol,
- ret = EFI_CALL(efi_install_multiple_protocol_interfaces
(&efi_root,/* Device path protocol */&efi_guid_device_path, dp,/* Device path to text protocol */&efi_guid_device_path_to_text_protocol,(void *)&efi_device_path_to_text,/* Device path utilities protocol */&efi_guid_device_path_utilities_protocol,(void *)&efi_device_path_utilities,/* Unicode collation protocol */&efi_guid_unicode_collation_protocol,(void *)&efi_unicode_collation_protocol,
/* HII string protocol */&efi_guid_hii_string_protocol,(void *)&efi_hii_string,/* HII database protocol */&efi_guid_hii_database_protocol,(void *)&efi_hii_database,/* HII configuration routing protocol */&efi_guid_hii_config_routing_protocol,(void *)&efi_hii_config_routing,
/* HII string protocol */&efi_guid_hii_string_protocol,(void *)&efi_hii_string,/* HII database protocol */&efi_guid_hii_database_protocol,(void *)&efi_hii_database,/* HII configuration routing protocol */&efi_guid_hii_config_routing_protocol,(void *)&efi_hii_config_routing,
NULL));
NULL));- efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
- return ret;
I guess this is part of the unrelated change?
Alex
}
2.20.1
On 5/6/19 9:24 AM, Graf, Alexander wrote:
On 06.05.19 07:42, Heinrich Schuchardt wrote:
If the file path is NULL, return EFI_INVALID_PARAMETER. If the file path is invalid, return EFI_NOT_FOUND.
These 2 are a documented inEFI_LOAD_FILE_PROTOCOL.LoadFile(). It might be a good idea to indicate that for later reference.
See the last sentence of the commit message.
If the size of the source buffer is 0, return EFI_LOAD_ERROR.
The spec says this should be EFI_INVALID_PARAMETER, no? Is this a spec or an SCT bug?
According to the UEFI 2.7A spec: EFI_LOAD_IMAGE should be returned if the image is corrupt.
UEFI SCT II Case Specification (June 2017), 3.4.1 LoadImage, Number 5.1.4.1.6
If the parent image handle does not refer to a loaded image return EFI_INVALID_PARAMETER.
I agree that this is a good change, but where is the spec reference? I couldn't find anything.
UEFI SCT II Case Specification (June 2017), 3.4.1 LoadImage, Number 5.1.4.1.1
The change is required to reach UEFI SCT conformance.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
IMHO this really should be a patch set with multiple individual changes, each changing one piece of spec conformance at a time.
That way if we happen to break anything along the way, bisecting would point us to exactly the right point where things fell apart.
v2 efi_load_image_from_path() must initalize *buffer even in case of an error
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 17 ++++++++---- lib/efi_loader/efi_root_node.c | 48 ++++++++++++++++++---------------- 3 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index d3a1d4c465..07ef14ba1c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -187,6 +187,7 @@ struct efi_handler { */ enum efi_object_type { EFI_OBJECT_TYPE_UNDEFINED = 0, + EFI_OBJECT_TYPE_U_BOOT_FIRMWARE,
This looks like an unrelated change?
No. We use efi_root as parent_image. We test if parent_image is an image in LoadImage. So efi_root must be marked as image.
EFI_OBJECT_TYPE_LOADED_IMAGE, EFI_OBJECT_TYPE_STARTED_IMAGE, }; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 78a4063949..f75e6c0169 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1680,10 +1680,13 @@ efi_status_t efi_load_image_from_path(struct efi_device_path *file_path, *buffer = NULL; *size = 0;
+ if (!file_path) + return EFI_INVALID_PARAMETER;
/* Open file */ f = efi_file_from_path(file_path); if (!f) - return EFI_DEVICE_ERROR; + return EFI_NOT_FOUND;
/* Get file size */ bs = 0; @@ -1760,13 +1763,13 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, EFI_ENTRY("%d, %p, %pD, %p, %zd, %p", boot_policy, parent_image, file_path, source_buffer, source_size, image_handle);
- if (!image_handle || !parent_image) { + if (!image_handle || !efi_search_obj(parent_image)) { ret = EFI_INVALID_PARAMETER; goto error; }
- if (!source_buffer && !file_path) { - ret = EFI_NOT_FOUND; + /* The parent image handle must refer to a loaded image */ + if (!parent_image->type) { + ret = EFI_INVALID_PARAMETER; goto error; }
@@ -1776,6 +1779,10 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, if (ret != EFI_SUCCESS) goto error; } else { + if (!source_size) { + ret = EFI_LOAD_ERROR; + goto error; + } dest_buffer = source_buffer; } /* split file_path which contains both the device and file parts */ diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c index e0fcbb85a4..38514e0820 100644 --- a/lib/efi_loader/efi_root_node.c +++ b/lib/efi_loader/efi_root_node.c @@ -28,6 +28,7 @@ struct efi_root_dp { */ efi_status_t efi_root_node_register(void) { + efi_status_t ret; struct efi_root_dp *dp;
/* Create device path protocol */ @@ -47,28 +48,31 @@ efi_status_t efi_root_node_register(void) dp->end.length = sizeof(struct efi_device_path);
/* Create root node and install protocols */ - return EFI_CALL(efi_install_multiple_protocol_interfaces(&efi_root, - /* Device path protocol */ - &efi_guid_device_path, dp, - /* Device path to text protocol */ - &efi_guid_device_path_to_text_protocol, - (void *)&efi_device_path_to_text, - /* Device path utilities protocol */ - &efi_guid_device_path_utilities_protocol, - (void *)&efi_device_path_utilities, - /* Unicode collation protocol */ - &efi_guid_unicode_collation_protocol, - (void *)&efi_unicode_collation_protocol, + ret = EFI_CALL(efi_install_multiple_protocol_interfaces + (&efi_root, + /* Device path protocol */ + &efi_guid_device_path, dp, + /* Device path to text protocol */ + &efi_guid_device_path_to_text_protocol, + (void *)&efi_device_path_to_text, + /* Device path utilities protocol */ + &efi_guid_device_path_utilities_protocol, + (void *)&efi_device_path_utilities, + /* Unicode collation protocol */ + &efi_guid_unicode_collation_protocol, + (void *)&efi_unicode_collation_protocol, #if CONFIG_IS_ENABLED(EFI_LOADER_HII) - /* HII string protocol */ - &efi_guid_hii_string_protocol, - (void *)&efi_hii_string, - /* HII database protocol */ - &efi_guid_hii_database_protocol, - (void *)&efi_hii_database, - /* HII configuration routing protocol */ - &efi_guid_hii_config_routing_protocol, - (void *)&efi_hii_config_routing, + /* HII string protocol */ + &efi_guid_hii_string_protocol, + (void *)&efi_hii_string, + /* HII database protocol */ + &efi_guid_hii_database_protocol, + (void *)&efi_hii_database, + /* HII configuration routing protocol */ + &efi_guid_hii_config_routing_protocol, + (void *)&efi_hii_config_routing, #endif - NULL)); + NULL)); + efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE; + return ret;
I guess this is part of the unrelated change?
No. We use efi_root as parent_image. We test if parent_image is an image in LoadImage. So efi_root must be marked as image.
Best regards
Heinrich
Alex
}
2.20.1
On 06.05.19 09:52, Heinrich Schuchardt wrote:
On 5/6/19 9:24 AM, Graf, Alexander wrote:
On 06.05.19 07:42, Heinrich Schuchardt wrote:
If the file path is NULL, return EFI_INVALID_PARAMETER. If the file path is invalid, return EFI_NOT_FOUND.
These 2 are a documented inEFI_LOAD_FILE_PROTOCOL.LoadFile(). It might be a good idea to indicate that for later reference.
See the last sentence of the commit message.
I don't understand? The return values are not defined as part of generic LoadImage(), but as part of EFI_LOAD_FILE_PROTOCOL.LoadFile() which is pretty counterintuitive IMHO.
If the size of the source buffer is 0, return EFI_LOAD_ERROR.
The spec says this should be EFI_INVALID_PARAMETER, no? Is this a spec or an SCT bug?
According to the UEFI 2.7A spec: EFI_LOAD_IMAGE should be returned if the image is corrupt.
UEFI SCT II Case Specification (June 2017), 3.4.1 LoadImage, Number 5.1.4.1.6
Hm, but LoadFile() explicitly says:
EFI_INVALID_PARAMETER | FilePath is not a valid device path, or BufferSize is NULL.
Or are we beyond that point here?
If the parent image handle does not refer to a loaded image return EFI_INVALID_PARAMETER.
I agree that this is a good change, but where is the spec reference? I couldn't find anything.
UEFI SCT II Case Specification (June 2017), 3.4.1 LoadImage, Number 5.1.4.1.1
Could you please add that reference to the commit message?
The change is required to reach UEFI SCT conformance.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
IMHO this really should be a patch set with multiple individual changes, each changing one piece of spec conformance at a time.
That way if we happen to break anything along the way, bisecting would point us to exactly the right point where things fell apart.
v2 efi_load_image_from_path() must initalize *buffer even in case of an error
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 17 ++++++++---- lib/efi_loader/efi_root_node.c | 48 ++++++++++++++++++---------------- 3 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index d3a1d4c465..07ef14ba1c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -187,6 +187,7 @@ struct efi_handler { */ enum efi_object_type { EFI_OBJECT_TYPE_UNDEFINED = 0, + EFI_OBJECT_TYPE_U_BOOT_FIRMWARE,
This looks like an unrelated change?
No. We use efi_root as parent_image. We test if parent_image is an image in LoadImage. So efi_root must be marked as image.
I see, that was not obvious :).
Alex
On 5/6/19 10:39 AM, Graf, Alexander wrote:
On 06.05.19 09:52, Heinrich Schuchardt wrote:
On 5/6/19 9:24 AM, Graf, Alexander wrote:
On 06.05.19 07:42, Heinrich Schuchardt wrote:
If the file path is NULL, return EFI_INVALID_PARAMETER.
There is a nice contradiction between UEFI Spec 2.7 Errata B and the UEFI SCT specfication.
The UEFI spec has: EFI_NOT_FOUND: Both SourceBuffer and DevicePath are NULL
UEFI SCT Specification (2017): 3.4.1 LoadImage, 5.1.4.1.2 BS.LoadImage – LoadImage() returns EFI_INVALID_PARAMETER with NULL FilePath.
If the file path is invalid, return EFI_NOT_FOUND.
UEFI Spec 2.7 Errata B: EFI_DEVICE_ERROR - Image was not loaded because the device returned a read error.
UEFI SCT Specification (2017): 3.4.1 LoadImage, 5.1.4.1.4 BS.LoadImage – LoadImage() returns EFI_NOT_FOUND with a non-existent FilePath.
These 2 are a documented inEFI_LOAD_FILE_PROTOCOL.LoadFile(). It might be a good idea to indicate that for later reference.
See the last sentence of the commit message.
I don't understand? The return values are not defined as part of generic LoadImage(), but as part of EFI_LOAD_FILE_PROTOCOL.LoadFile() which is pretty counterintuitive IMHO.
The definitions in the EFI_LOAD_FILE_PROTOCOL should not prescribe the return values of LoadImage().
I guess we should stick with the UEFI spec in case of contradiction and try to convince uefi.org that there is a bug in the SCT.
Best regards
Heinrich
If the size of the source buffer is 0, return EFI_LOAD_ERROR.
The spec says this should be EFI_INVALID_PARAMETER, no? Is this a spec or an SCT bug?
According to the UEFI 2.7A spec: EFI_LOAD_IMAGE should be returned if the image is corrupt.
UEFI SCT II Case Specification (June 2017), 3.4.1 LoadImage, Number 5.1.4.1.6
Hm, but LoadFile() explicitly says:
EFI_INVALID_PARAMETER | FilePath is not a valid device path, or BufferSize is NULL.
Or are we beyond that point here?
If the parent image handle does not refer to a loaded image return EFI_INVALID_PARAMETER.
I agree that this is a good change, but where is the spec reference? I couldn't find anything.
UEFI SCT II Case Specification (June 2017), 3.4.1 LoadImage, Number 5.1.4.1.1
Could you please add that reference to the commit message?
The change is required to reach UEFI SCT conformance.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
IMHO this really should be a patch set with multiple individual changes, each changing one piece of spec conformance at a time.
That way if we happen to break anything along the way, bisecting would point us to exactly the right point where things fell apart.
v2 efi_load_image_from_path() must initalize *buffer even in case of an error
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 17 ++++++++---- lib/efi_loader/efi_root_node.c | 48 ++++++++++++++++++---------------- 3 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index d3a1d4c465..07ef14ba1c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -187,6 +187,7 @@ struct efi_handler { */ enum efi_object_type { EFI_OBJECT_TYPE_UNDEFINED = 0, + EFI_OBJECT_TYPE_U_BOOT_FIRMWARE,
This looks like an unrelated change?
No. We use efi_root as parent_image. We test if parent_image is an image in LoadImage. So efi_root must be marked as image.
I see, that was not obvious :).
Alex
On 06.05.19 13:32, Heinrich Schuchardt wrote:
On 5/6/19 10:39 AM, Graf, Alexander wrote:
On 06.05.19 09:52, Heinrich Schuchardt wrote:
On 5/6/19 9:24 AM, Graf, Alexander wrote:
On 06.05.19 07:42, Heinrich Schuchardt wrote:
If the file path is NULL, return EFI_INVALID_PARAMETER.
There is a nice contradiction between UEFI Spec 2.7 Errata B and the UEFI SCT specfication.
The UEFI spec has: EFI_NOT_FOUND: Both SourceBuffer and DevicePath are NULL
UEFI SCT Specification (2017): 3.4.1 LoadImage, 5.1.4.1.2 BS.LoadImage – LoadImage() returns EFI_INVALID_PARAMETER with NULL FilePath.
If the file path is invalid, return EFI_NOT_FOUND.
UEFI Spec 2.7 Errata B: EFI_DEVICE_ERROR - Image was not loaded because the device returned a read error.
UEFI SCT Specification (2017): 3.4.1 LoadImage, 5.1.4.1.4 BS.LoadImage – LoadImage() returns EFI_NOT_FOUND with a non-existent FilePath.
These 2 are a documented inEFI_LOAD_FILE_PROTOCOL.LoadFile(). It might be a good idea to indicate that for later reference.
See the last sentence of the commit message.
I don't understand? The return values are not defined as part of generic LoadImage(), but as part of EFI_LOAD_FILE_PROTOCOL.LoadFile() which is pretty counterintuitive IMHO.
The definitions in the EFI_LOAD_FILE_PROTOCOL should not prescribe the return values of LoadImage().
I guess we should stick with the UEFI spec in case of contradiction and try to convince uefi.org that there is a bug in the SCT.
Vincent, do you have any further input for us here? :)
Thanks!
Alex
Best regards
Heinrich
If the size of the source buffer is 0, return EFI_LOAD_ERROR.
The spec says this should be EFI_INVALID_PARAMETER, no? Is this a spec or an SCT bug?
According to the UEFI 2.7A spec: EFI_LOAD_IMAGE should be returned if the image is corrupt.
UEFI SCT II Case Specification (June 2017), 3.4.1 LoadImage, Number 5.1.4.1.6
Hm, but LoadFile() explicitly says:
EFI_INVALID_PARAMETER | FilePath is not a valid device path, or BufferSize is NULL.
Or are we beyond that point here?
If the parent image handle does not refer to a loaded image return EFI_INVALID_PARAMETER.
I agree that this is a good change, but where is the spec reference? I couldn't find anything.
UEFI SCT II Case Specification (June 2017), 3.4.1 LoadImage, Number 5.1.4.1.1
Could you please add that reference to the commit message?
The change is required to reach UEFI SCT conformance.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
IMHO this really should be a patch set with multiple individual changes, each changing one piece of spec conformance at a time.
That way if we happen to break anything along the way, bisecting would point us to exactly the right point where things fell apart.
v2 efi_load_image_from_path() must initalize *buffer even in case of an error
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 17 ++++++++---- lib/efi_loader/efi_root_node.c | 48 ++++++++++++++++++---------------- 3 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index d3a1d4c465..07ef14ba1c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -187,6 +187,7 @@ struct efi_handler { */ enum efi_object_type { EFI_OBJECT_TYPE_UNDEFINED = 0, + EFI_OBJECT_TYPE_U_BOOT_FIRMWARE,
This looks like an unrelated change?
No. We use efi_root as parent_image. We test if parent_image is an image in LoadImage. So efi_root must be marked as image.
I see, that was not obvious :).
Alex
participants (2)
-
Graf, Alexander -
Heinrich Schuchardt