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