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

[Heinrich Schuchardt]

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

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
>