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.g...@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 >> > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot