| Message ID | 1516747599-16401-1-git-send-email-xypron.glpk@gmx.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [U-Boot,1/1] efi_loader: always call Exit after an image returns. | expand |
On 23.01.18 23:46, Heinrich Schuchardt wrote: > If an application or driver started via StartImage returns without > calling Exit, StartImage has to call Exit. This is mandated by the > UEFI spec and we do the same in efi_do_enter(). > > The patch looks bigger than it is. To avoid a forward declaration function > efi_exit() was moved up. Only one (void*) was replaced by (void *). > The only real change is in efi_start_image(). > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > Without this patch on Ubuntu 16.04 a crash could be observed in > efi_selftest_startimage_return.c. With the patch it is gone. > But this could be coincidence. > --- > lib/efi_loader/efi_boottime.c | 99 ++++++++++++++++++++++--------------------- > 1 file changed, 51 insertions(+), 48 deletions(-) > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 0ac5b44..62a24bf 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -1519,6 +1519,54 @@ failure: > } > > /* > + * Leave an EFI application or driver. > + * > + * This function implements the Exit service. > + * See the Unified Extensible Firmware Interface (UEFI) specification > + * for details. > + * > + * @image_handle handle of the application or driver that is exiting > + * @exit_status status code > + * @exit_data_size size of the buffer in bytes > + * @exit_data buffer with data describing an error > + * @return status code > + */ > +static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, > + efi_status_t exit_status, unsigned long exit_data_size, > + int16_t *exit_data) > +{ > + /* > + * We require that the handle points to the original loaded > + * image protocol interface. > + * > + * For getting the longjmp address this is safer than locating > + * the protocol because the protocol may have been reinstalled > + * pointing to another memory location. > + * > + * TODO: We should call the unload procedure of the loaded > + * image protocol. > + */ > + struct efi_loaded_image *loaded_image_info = (void *)image_handle; > + > + EFI_ENTRY("%p, %ld, %ld, %p", image_handle, exit_status, > + exit_data_size, exit_data); > + > + /* Make sure entry/exit counts for EFI world cross-overs match */ > + EFI_EXIT(exit_status); > + > + /* > + * But longjmp out with the U-Boot gd, not the application's, as > + * the other end is a setjmp call inside EFI context. > + */ > + efi_restore_gd(); > + > + loaded_image_info->exit_status = exit_status; > + longjmp(&loaded_image_info->exit_jmp, 1); > + > + panic("EFI application exited"); > +} > + > +/* > * Call the entry point of an image. > * > * This function implements the StartImage service. > @@ -1575,59 +1623,14 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, > > ret = EFI_CALL(entry(image_handle, &systab)); > > + /* Clean up the image */ > + ret = EFI_CALL(efi_exit(image_handle, ret, 0, NULL)); Just use the systab here, like we do in efi_do_enter(). While I think the change is reasonable, as it brings the bootefi and the start_image calls together, I can't really see an obvious reason why this code would work and the one without longjmp would not... Alex
On 01/24/2018 12:31 AM, Alexander Graf wrote: > > > On 23.01.18 23:46, Heinrich Schuchardt wrote: >> If an application or driver started via StartImage returns without >> calling Exit, StartImage has to call Exit. This is mandated by the >> UEFI spec and we do the same in efi_do_enter(). >> >> The patch looks bigger than it is. To avoid a forward declaration function >> efi_exit() was moved up. Only one (void*) was replaced by (void *). >> The only real change is in efi_start_image(). >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> Without this patch on Ubuntu 16.04 a crash could be observed in >> efi_selftest_startimage_return.c. With the patch it is gone. >> But this could be coincidence. >> --- >> lib/efi_loader/efi_boottime.c | 99 ++++++++++++++++++++++--------------------- >> 1 file changed, 51 insertions(+), 48 deletions(-) >> >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >> index 0ac5b44..62a24bf 100644 >> --- a/lib/efi_loader/efi_boottime.c >> +++ b/lib/efi_loader/efi_boottime.c >> @@ -1519,6 +1519,54 @@ failure: >> } >> >> /* >> + * Leave an EFI application or driver. >> + * >> + * This function implements the Exit service. >> + * See the Unified Extensible Firmware Interface (UEFI) specification >> + * for details. >> + * >> + * @image_handle handle of the application or driver that is exiting >> + * @exit_status status code >> + * @exit_data_size size of the buffer in bytes >> + * @exit_data buffer with data describing an error >> + * @return status code >> + */ >> +static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, >> + efi_status_t exit_status, unsigned long exit_data_size, >> + int16_t *exit_data) >> +{ >> + /* >> + * We require that the handle points to the original loaded >> + * image protocol interface. >> + * >> + * For getting the longjmp address this is safer than locating >> + * the protocol because the protocol may have been reinstalled >> + * pointing to another memory location. >> + * >> + * TODO: We should call the unload procedure of the loaded >> + * image protocol. >> + */ >> + struct efi_loaded_image *loaded_image_info = (void *)image_handle; >> + >> + EFI_ENTRY("%p, %ld, %ld, %p", image_handle, exit_status, >> + exit_data_size, exit_data); >> + >> + /* Make sure entry/exit counts for EFI world cross-overs match */ >> + EFI_EXIT(exit_status); >> + >> + /* >> + * But longjmp out with the U-Boot gd, not the application's, as >> + * the other end is a setjmp call inside EFI context. >> + */ >> + efi_restore_gd(); >> + >> + loaded_image_info->exit_status = exit_status; >> + longjmp(&loaded_image_info->exit_jmp, 1); >> + >> + panic("EFI application exited"); >> +} >> + >> +/* >> * Call the entry point of an image. >> * >> * This function implements the StartImage service. >> @@ -1575,59 +1623,14 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, >> >> ret = EFI_CALL(entry(image_handle, &systab)); >> >> + /* Clean up the image */ >> + ret = EFI_CALL(efi_exit(image_handle, ret, 0, NULL)); > > Just use the systab here, like we do in efi_do_enter(). No, efi_do_enter passes image_handle to efi_exit(). The UEFI API requires the image handle. It is the same image handle that the application would pass when calling Exit. efi_exit() uses the handle as pointer to the loaded image information. > > While I think the change is reasonable, as it brings the bootefi and the > start_image calls together, I can't really see an obvious reason why > this code would work and the one without longjmp would not... Yes analyzing the crash observed on Ubuntu is a different story. This shouldn't stop us from making efi_start_image standard compliant. Maybe you want to put this patch into an branch for 2018.05-rc1. Regards Heinrich > > > Alex >
> Am 24.01.2018 um 01:05 schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>: > >> On 01/24/2018 12:31 AM, Alexander Graf wrote: >> >> >>> On 23.01.18 23:46, Heinrich Schuchardt wrote: >>> If an application or driver started via StartImage returns without >>> calling Exit, StartImage has to call Exit. This is mandated by the >>> UEFI spec and we do the same in efi_do_enter(). >>> >>> The patch looks bigger than it is. To avoid a forward declaration function >>> efi_exit() was moved up. Only one (void*) was replaced by (void *). >>> The only real change is in efi_start_image(). >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> --- >>> Without this patch on Ubuntu 16.04 a crash could be observed in >>> efi_selftest_startimage_return.c. With the patch it is gone. >>> But this could be coincidence. >>> --- >>> lib/efi_loader/efi_boottime.c | 99 ++++++++++++++++++++++--------------------- >>> 1 file changed, 51 insertions(+), 48 deletions(-) >>> >>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >>> index 0ac5b44..62a24bf 100644 >>> --- a/lib/efi_loader/efi_boottime.c >>> +++ b/lib/efi_loader/efi_boottime.c >>> @@ -1519,6 +1519,54 @@ failure: >>> } >>> >>> /* >>> + * Leave an EFI application or driver. >>> + * >>> + * This function implements the Exit service. >>> + * See the Unified Extensible Firmware Interface (UEFI) specification >>> + * for details. >>> + * >>> + * @image_handle handle of the application or driver that is exiting >>> + * @exit_status status code >>> + * @exit_data_size size of the buffer in bytes >>> + * @exit_data buffer with data describing an error >>> + * @return status code >>> + */ >>> +static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, >>> + efi_status_t exit_status, unsigned long exit_data_size, >>> + int16_t *exit_data) >>> +{ >>> + /* >>> + * We require that the handle points to the original loaded >>> + * image protocol interface. >>> + * >>> + * For getting the longjmp address this is safer than locating >>> + * the protocol because the protocol may have been reinstalled >>> + * pointing to another memory location. >>> + * >>> + * TODO: We should call the unload procedure of the loaded >>> + * image protocol. >>> + */ >>> + struct efi_loaded_image *loaded_image_info = (void *)image_handle; >>> + >>> + EFI_ENTRY("%p, %ld, %ld, %p", image_handle, exit_status, >>> + exit_data_size, exit_data); >>> + >>> + /* Make sure entry/exit counts for EFI world cross-overs match */ >>> + EFI_EXIT(exit_status); >>> + >>> + /* >>> + * But longjmp out with the U-Boot gd, not the application's, as >>> + * the other end is a setjmp call inside EFI context. >>> + */ >>> + efi_restore_gd(); >>> + >>> + loaded_image_info->exit_status = exit_status; >>> + longjmp(&loaded_image_info->exit_jmp, 1); >>> + >>> + panic("EFI application exited"); >>> +} >>> + >>> +/* >>> * Call the entry point of an image. >>> * >>> * This function implements the StartImage service. >>> @@ -1575,59 +1623,14 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, >>> >>> ret = EFI_CALL(entry(image_handle, &systab)); >>> >>> + /* Clean up the image */ >>> + ret = EFI_CALL(efi_exit(image_handle, ret, 0, NULL)); >> >> Just use the systab here, like we do in efi_do_enter(). > > No, efi_do_enter passes image_handle to efi_exit(). > > The UEFI API requires the image handle. It is the same image handle that > the application would pass when calling Exit. > > efi_exit() uses the handle as pointer to the loaded image information. I mean use the systab to get to efi_exit instead of shuffling code around :). It‘s what we do in bootefi as well. Alex
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 0ac5b44..62a24bf 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1519,6 +1519,54 @@ failure: } /* + * Leave an EFI application or driver. + * + * This function implements the Exit service. + * See the Unified Extensible Firmware Interface (UEFI) specification + * for details. + * + * @image_handle handle of the application or driver that is exiting + * @exit_status status code + * @exit_data_size size of the buffer in bytes + * @exit_data buffer with data describing an error + * @return status code + */ +static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, + efi_status_t exit_status, unsigned long exit_data_size, + int16_t *exit_data) +{ + /* + * We require that the handle points to the original loaded + * image protocol interface. + * + * For getting the longjmp address this is safer than locating + * the protocol because the protocol may have been reinstalled + * pointing to another memory location. + * + * TODO: We should call the unload procedure of the loaded + * image protocol. + */ + struct efi_loaded_image *loaded_image_info = (void *)image_handle; + + EFI_ENTRY("%p, %ld, %ld, %p", image_handle, exit_status, + exit_data_size, exit_data); + + /* Make sure entry/exit counts for EFI world cross-overs match */ + EFI_EXIT(exit_status); + + /* + * But longjmp out with the U-Boot gd, not the application's, as + * the other end is a setjmp call inside EFI context. + */ + efi_restore_gd(); + + loaded_image_info->exit_status = exit_status; + longjmp(&loaded_image_info->exit_jmp, 1); + + panic("EFI application exited"); +} + +/* * Call the entry point of an image. * * This function implements the StartImage service. @@ -1575,59 +1623,14 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, ret = EFI_CALL(entry(image_handle, &systab)); + /* Clean up the image */ + ret = EFI_CALL(efi_exit(image_handle, ret, 0, NULL)); + /* Should usually never get here */ return EFI_EXIT(ret); } /* - * Leave an EFI application or driver. - * - * This function implements the Exit service. - * See the Unified Extensible Firmware Interface (UEFI) specification - * for details. - * - * @image_handle handle of the application or driver that is exiting - * @exit_status status code - * @exit_data_size size of the buffer in bytes - * @exit_data buffer with data describing an error - * @return status code - */ -static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, - efi_status_t exit_status, unsigned long exit_data_size, - int16_t *exit_data) -{ - /* - * We require that the handle points to the original loaded - * image protocol interface. - * - * For getting the longjmp address this is safer than locating - * the protocol because the protocol may have been reinstalled - * pointing to another memory location. - * - * TODO: We should call the unload procedure of the loaded - * image protocol. - */ - struct efi_loaded_image *loaded_image_info = (void*)image_handle; - - EFI_ENTRY("%p, %ld, %ld, %p", image_handle, exit_status, - exit_data_size, exit_data); - - /* Make sure entry/exit counts for EFI world cross-overs match */ - EFI_EXIT(exit_status); - - /* - * But longjmp out with the U-Boot gd, not the application's, as - * the other end is a setjmp call inside EFI context. - */ - efi_restore_gd(); - - loaded_image_info->exit_status = exit_status; - longjmp(&loaded_image_info->exit_jmp, 1); - - panic("EFI application exited"); -} - -/* * Unload an EFI image. * * This function implements the UnloadImage service.
If an application or driver started via StartImage returns without calling Exit, StartImage has to call Exit. This is mandated by the UEFI spec and we do the same in efi_do_enter(). The patch looks bigger than it is. To avoid a forward declaration function efi_exit() was moved up. Only one (void*) was replaced by (void *). The only real change is in efi_start_image(). Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- Without this patch on Ubuntu 16.04 a crash could be observed in efi_selftest_startimage_return.c. With the patch it is gone. But this could be coincidence. --- lib/efi_loader/efi_boottime.c | 99 ++++++++++++++++++++++--------------------- 1 file changed, 51 insertions(+), 48 deletions(-)