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

[Heinrich Schuchardt]

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

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
>>
>