[U-Boot] [PATCH 1/1] efi_loader: always call Exit after an image returns.

[Alexander Graf]

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