[U-Boot] [PATCH v2 1/1] efi_loader: LoadImage() parameter checks

[Graf, Alexander]

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 at 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;
>   		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?



Alex



>   }
> --
> 2.20.1
>