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

[Graf, Alexander]

On 06.05.19 13:32, Heinrich Schuchardt wrote:
>
> 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.


Vincent, do you have any further input for us here? :)


Thanks!

Alex


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