Re: [PATCH v2 07/10] efi: payload: pass shell arguments as load options to executed images

[Marco Felsch <m.felsch@pengutronix.de>]

On 26-10-15 11:18, Ahmad Fatoum wrote:
> EFI applications executed directly from the shell via the binfmt hook,
> e.g. "/boot/shell.efi -nostartup", currently have their arguments silently
> dropped: the only load options ever set are the Linux bootargs and only if
> the image was detected as an EFI-stubbed kernel.
> 
> Serialize the arguments following the image path into the load options
> instead and leave it to the caller of efi_execute_image() to decide what
> the load options should be. bootm keeps passing the Linux bootargs for
> kernel images, but executing a kernel image directly from the shell now
> passes exactly what was typed on the command line, which makes the
> documentation's claim that only bootm passes the kernel command line true
> again.
> 
> The firmware unloads an application as soon as it returns, as does barebox'
> own loader in efi_exit() and EDK2 in CoreStartImage(), so the load options
> may only be cleared for a driver that started successfully and is thus
> still around. The buffer itself belongs to barebox and is freed either way.
> 
> While at it, add the missing space in the "Booting kernel via StartImage
> with options" message.
> 
> Assisted-by: Claude:fable-5
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  efi/payload/bootm.c |  9 ++++++--
>  efi/payload/image.c | 51 ++++++++++++++++++++++++++++++++++-----------
>  efi/payload/image.h |  2 +-
>  3 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
> index 2bcfd90e42fa..fe2d27b7ff10 100644
> --- a/efi/payload/bootm.c
> +++ b/efi/payload/bootm.c
> @@ -22,6 +22,7 @@
>  #include <string.h>
>  #include <linux/err.h>
>  #include <boot.h>
> +#include <bootargs.h>
>  #include <bootm.h>
>  #include <fs.h>
>  #include <libfile.h>
> @@ -185,7 +186,9 @@ static int do_bootm_efi_stub(struct image_data *data)
>  	if (data->dryrun)
>  		goto unload_ramdisk;
>  
> -	ret = efi_execute_image(handle, loaded_image, true, type);
> +	ret = efi_execute_image(handle, loaded_image, true, type,
> +				filetype_is_linux_efi_image(type) ?
> +				linux_bootargs_get() : NULL);
>  
>  	/* efi_execute_image takes care to unload the image on error,
>  	 * so we set image_freed and fall through to freeing ramdisk
> @@ -225,7 +228,9 @@ static int efi_app_execute(struct image_data *data)
>  		return 0;
>  	}
>  
> -	return efi_execute_image(handle, loaded_image, true, type);
> +	return efi_execute_image(handle, loaded_image, true, type,
> +				 filetype_is_linux_efi_image(type) ?
> +				 linux_bootargs_get() : NULL);
>  }
>  
>  static int linux_efi_handover = true;
> diff --git a/efi/payload/image.c b/efi/payload/image.c
> index 3668a6a04cac..445eaaa587d9 100644
> --- a/efi/payload/image.c
> +++ b/efi/payload/image.c
> @@ -18,7 +18,6 @@
>  #include <malloc.h>
>  #include <string.h>
>  #include <linux/err.h>
> -#include <bootargs.h>
>  #include <bootm.h>
>  #include <fs.h>
>  #include <libfile.h>
> @@ -101,10 +100,10 @@ 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)
> +		      enum filetype filetype, const char *options)
>  {
>  	efi_status_t efiret;
> -	const char *options;
> +	wchar_t *load_options = NULL;
>  	bool is_driver;
>  	bool no_return;
>  
> @@ -118,16 +117,20 @@ int efi_execute_image(efi_handle_t handle,
>  	 */
>  	no_return = is_bootm || filetype_is_linux_efi_image(filetype);
>  
> +	if (options && *options) {
> +		load_options = xstrdup_char_to_wchar(options);
> +		loaded_image->load_options = load_options;
> +		loaded_image->load_options_size =
> +			(strlen(options) + 1) * sizeof(wchar_t);
> +	}
> +
>  	if (filetype_is_linux_efi_image(filetype)) {
> -		options = linux_bootargs_get();
>  		printf("Booting kernel via StartImage");
> -		if (options) {
> -			printf("with options '%s'", options);
> -			loaded_image->load_options = xstrdup_char_to_wchar(options);
> -			loaded_image->load_options_size =
> -				(strlen(options) + 1) * sizeof(wchar_t);
> -		}
> +		if (load_options)
> +			printf(" with options '%s'", options);
>  		printf("...\n");
> +	} else if (load_options) {
> +		pr_debug("Starting image with options '%s'\n", options);
>  	}

Can we please make use one print style either printf() or pr_*()? IMHO
pr_*() is better since this goes into the log buffer which could be
pstore backed.

Furthermore this if-else now is only responsible for handling the print
message, can we make this cleaner?

>  
>  	if (no_return) {
> @@ -150,8 +153,24 @@ int efi_execute_image(efi_handle_t handle,
>  	if (EFI_ERROR(efiret))
>  		pr_err("failed to StartImage: %s\n", efi_strerror(efiret));
>  
> -	if (!is_driver)
> +	/*
> +	 * The firmware unloads an application as soon as it returns, as well as
> +	 * a driver that failed to start, freeing the loaded image protocol with
> +	 * it. Only a still loaded driver's protocol may be touched here, and it
> +	 * must be, as it references the load options we are about to free.
> +	 * Unloading an application that already returned just fails, but is
> +	 * still needed when StartImage failed before running it.
> +	 */
> +	if (is_driver) {
> +		if (!EFI_ERROR(efiret)) {
> +			loaded_image->load_options = NULL;
					^
Don't we have to free this first?

Regards,
  Marco

> +			loaded_image->load_options_size = 0;
> +		}
> +	} else {
>  		BS->unload_image(handle);
> +	}
> +
> +	free(load_options);
>  
>  	efi_connect_all();
>  	efi_register_devices();
> @@ -163,13 +182,21 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
>  {
>  	struct efi_loaded_image *loaded_image;
>  	efi_handle_t handle;
> +	char *options;
>  	int ret;
>  
>  	ret = efi_load_image(file, &loaded_image, &handle);
>  	if (ret)
>  		return ret;
>  
> -	return efi_execute_image(handle, loaded_image, false, b->type);
> +	/* argv[0] is the image itself, the rest become the load options */
> +	options = strjoin(" ", &argv[1], argc - 1);
> +
> +	ret = efi_execute_image(handle, loaded_image, false, b->type, options);
> +
> +	free(options);
> +
> +	return ret;
>  }
>  
>  static struct binfmt_hook binfmt_efi_hook = {
> diff --git a/efi/payload/image.h b/efi/payload/image.h
> index 33f7e1a21b30..54494c71626a 100644
> --- a/efi/payload/image.h
> +++ b/efi/payload/image.h
> @@ -14,7 +14,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);
> +		      enum filetype filetype, const char *options);
>  
>  extern struct image_handler efi_x86_linux_handle_tr;
>  extern struct image_handler efi_x86_linux_handle_handover;
> -- 
> 2.47.3
> 
>