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 <[email protected]>
> ---
> 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
>
>
