[U-Boot] [RFC 1/8] efi_loader: boottime: don't add device path protocol to image handle

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, Mar 06, 2019 at 06:29:14AM +0100, Heinrich Schuchardt wrote:
> On 3/6/19 6:04 AM, Heinrich Schuchardt wrote:
> > On 3/6/19 1:27 AM, AKASHI Takahiro wrote:
> >> On Tue, Mar 05, 2019 at 08:48:37PM +0100, Heinrich Schuchardt wrote:
> >>> On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
> >>>> It is just wrong to add devcie path protocol to image handle.
> >>>>
> >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>> ---
> >>>>  lib/efi_loader/efi_boottime.c | 11 +----------
> >>>>  1 file changed, 1 insertion(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >>>> index bd8b8a17ae71..7bd9c0a952d4 100644
> >>>> --- a/lib/efi_loader/efi_boottime.c
> >>>> +++ b/lib/efi_loader/efi_boottime.c
> >>>> @@ -1540,17 +1540,8 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >>>>  	info->file_path = file_path;
> >>>>  	info->system_table = &systab;
> >>>>  
> >>>> -	if (device_path) {
> >>>> +	if (device_path)
> >>>>  		info->device_handle = efi_dp_find_obj(device_path, NULL);
> >>>> -		/*
> >>>> -		 * When asking for the device path interface, return
> >>>> -		 * bootefi_device_path
> >>>> -		 */
> >>>> -		ret = efi_add_protocol(&obj->header,
> >>>> -				       &efi_guid_device_path, device_path);
> >>>
> >>> Installing the device path is not the problem. It is the GUID that is
> >>> wrong. Use EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID here.
> >>
> >> Okay, but I believe that we need duplicate device_path here
> >> before installing it as EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID.
> >>
> >> See this line:
> >>
> >>>>  		info->device_handle = efi_dp_find_obj(device_path, NULL);
> >>
> >> Normally, device_path is expected to be already associated with
> >> another handle. We should not allow two handles to share one protocol(data).
> >> That is also why I initially believed that add_protocol() should be removed.
> > 
> > The spec says we should use a copy of the unchanged DevicePath parameter
> > of LoadImage() which may be NULL.
> > 
> > So we have to rework all callers to get the device_path parameter of
> > efi_setup_loaded_image() right.
> > 
> 
> Why are we constructing a dummy memory device path at all in cmd/bootefi?
> 
> The commit message of patch bf19273e81eb "efi_loader: Add mem-mapped for
> fallback" that introduced this does not give a valid answer as it is
> explicitly allowable to call LoadImage with DevicePath = NULL if
> SourceBuffer is provided.

As far as I know, if we load EDK2's Shell.efi by calling LoadImage
*without* DevicePath, it will fail to boot at some assertion check.

-Takahiro Akashi

> So I suggest we rid ourselves of the dummy device path with this patch
> series.
> 
> Best regards
> 
> Heinrich