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

[Heinrich Schuchardt]

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 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.
>
>
> I see, that was not obvious :).
>
> Alex
>
>
>