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

[Graf, Alexander]

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


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.


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