| Message ID | 20190305055337.3793-2-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: rework bootefi/bootmgr | expand |
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. UEFI Spec 2.7: "The Loaded Image Device Path Protocol must be installed onto the image handle of a PE/COFF image loaded through the EFI Boot Service LoadImage()." Best regards Heinrich > - if (ret != EFI_SUCCESS) > - goto failure; > - } > > /* > * When asking for the loaded_image interface, just >
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. Thanks, -Takahiro Akashi > UEFI Spec 2.7: > > "The Loaded Image Device Path Protocol must be installed onto the image > handle of a PE/COFF image loaded through the EFI Boot Service LoadImage()." > > Best regards > > Heinrich > > > - if (ret != EFI_SUCCESS) > > - goto failure; > > - } > > > > /* > > * When asking for the loaded_image interface, just > > >
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. Best regards Heinrich > > Thanks, > -Takahiro Akashi > > >> UEFI Spec 2.7: >> >> "The Loaded Image Device Path Protocol must be installed onto the image >> handle of a PE/COFF image loaded through the EFI Boot Service LoadImage()." >> >> Best regards >> >> Heinrich >> >>> - if (ret != EFI_SUCCESS) >>> - goto failure; >>> - } >>> >>> /* >>> * When asking for the loaded_image interface, just >>> >> >
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. So I suggest we rid ourselves of the dummy device path with this patch series. Best regards Heinrich
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
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); - if (ret != EFI_SUCCESS) - goto failure; - } /* * When asking for the loaded_image interface, just
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(-)