Re: [PATCH v2 06/10] efi: payload: always shutdown barebox when booting

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 10/15/26 12:03 PM, Marco Felsch wrote:
> Hi Ahmad,
> 
> On 26-10-15 11:18, Ahmad Fatoum wrote:
>> When barebox is about to boot a kernel image, it calls shutdown_barebox
>> and then starts the image. When the kernel image is wrapped in a UKI,
>> barebox will not detect it as a kernel image and will thus not call
>> shutdown_barebox beforehand, which can mean that e.g. state is not
>> flushed.
>>
>> We do not want to treat UKIs completely like kernels (e.g. we do not
>> want to override their built-in bootargs), but we still want to properly
>> shutdown barebox.
> 
> But it's still possible to pass further kernel commandline arguments to
> an UKI?

Only by explicit decision via global.efi.bootargs added in a later
commit. Quoting systemd-stub man page:

If UEFI SecureBoot is enabled and the ".cmdline" section is present in
the executed image, any attempts to override the kernel command line by
passing one as invocation parameters to the EFI binary are ignored.
Thus, in order to allow overriding the kernel command line, either
disable UEFI SecureBoot, or do not include a kernel command line PE
section in the kernel image file.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>>
>> Resolve this by shutting down barebox whenever we are in a bootm handler,
>> no matter which kind of image is about to be started: bootm is the point
>> of no return. Images run from the shell via binfmt keep returning to
>> barebox afterwards, with the exception of EFI-stubbed kernels, which take
>> over the machine and thus continue to shut barebox down as before.
>>
>> Assisted-by: Claude:opus-5
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  efi/payload/bootm.c |  4 ++--
>>  efi/payload/image.c | 21 +++++++++++++++------
>>  efi/payload/image.h |  1 +
>>  3 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
>> index 3168e07a3dc1..2bcfd90e42fa 100644
>> --- a/efi/payload/bootm.c
>> +++ b/efi/payload/bootm.c
>> @@ -185,7 +185,7 @@ static int do_bootm_efi_stub(struct image_data *data)
>>  	if (data->dryrun)
>>  		goto unload_ramdisk;
>>  
>> -	ret = efi_execute_image(handle, loaded_image, type);
>> +	ret = efi_execute_image(handle, loaded_image, true, type);
>>  
>>  	/* efi_execute_image takes care to unload the image on error,
>>  	 * so we set image_freed and fall through to freeing ramdisk
>> @@ -225,7 +225,7 @@ static int efi_app_execute(struct image_data *data)
>>  		return 0;
>>  	}
>>  
>> -	return efi_execute_image(handle, loaded_image, type);
>> +	return efi_execute_image(handle, loaded_image, true, type);
>>  }
>>  
>>  static int linux_efi_handover = true;
>> diff --git a/efi/payload/image.c b/efi/payload/image.c
>> index 378709b6de95..3668a6a04cac 100644
>> --- a/efi/payload/image.c
>> +++ b/efi/payload/image.c
>> @@ -100,15 +100,24 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>>  
>>  int efi_execute_image(efi_handle_t handle,
>>  		      struct efi_loaded_image *loaded_image,
>> +		      bool is_bootm,
>>  		      enum filetype filetype)
>>  {
>>  	efi_status_t efiret;
>>  	const char *options;
>> -	bool is_driver, is_kernel = false;
>> +	bool is_driver;
>> +	bool no_return;
>>  
>>  	is_driver = (loaded_image->image_code_type == EFI_BOOT_SERVICES_CODE) ||
>>  		(loaded_image->image_code_type == EFI_RUNTIME_SERVICES_CODE);
>>  
>> +	/*
>> +	 * A bootm handler is the point of no return, but an EFI-stubbed kernel
>> +	 * started from the shell takes over the machine just the same, so
>> +	 * barebox needs to be shut down in both cases.
>> +	 */
>> +	no_return = is_bootm || filetype_is_linux_efi_image(filetype);
>> +
>>  	if (filetype_is_linux_efi_image(filetype)) {
>>  		options = linux_bootargs_get();
>>  		printf("Booting kernel via StartImage");
>> @@ -119,11 +128,11 @@ int efi_execute_image(efi_handle_t handle,
>>  				(strlen(options) + 1) * sizeof(wchar_t);
>>  		}
>>  		printf("...\n");
>> +	}
>>  
>> +	if (no_return) {
>>  		efi_set_variable_usec("LoaderTimeExecUSec", &efi_systemd_vendor_guid,
>>  				      ktime_to_us(ktime_get()));
>> -
>> -		is_kernel = true;
>>  		shutdown_barebox();
>>  	}
>>  
>> @@ -133,8 +142,8 @@ int efi_execute_image(efi_handle_t handle,
>>  
>>  	efi_continue_devices();
>>  
>> -	if (is_kernel) {
>> -		pr_emerg("Kernel image has unexpectedly returned\n");
>> +	if (no_return) {
>> +		pr_emerg("Boot image has unexpectedly returned\n");
>>  		BS->exit(efi_parent_image, efiret, 0, NULL);
>>  	}
>>  
>> @@ -160,7 +169,7 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
>>  	if (ret)
>>  		return ret;
>>  
>> -	return efi_execute_image(handle, loaded_image, b->type);
>> +	return efi_execute_image(handle, loaded_image, false, b->type);
>>  }
>>  
>>  static struct binfmt_hook binfmt_efi_hook = {
>> diff --git a/efi/payload/image.h b/efi/payload/image.h
>> index bab1be368c21..33f7e1a21b30 100644
>> --- a/efi/payload/image.h
>> +++ b/efi/payload/image.h
>> @@ -13,6 +13,7 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>>  
>>  int efi_execute_image(efi_handle_t handle,
>>  		      struct efi_loaded_image *loaded_image,
>> +		      bool is_bootm,
>>  		      enum filetype filetype);
>>  
>>  extern struct image_handler efi_x86_linux_handle_tr;
>> -- 
>> 2.47.3
>>
>>

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |