grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] efi: Copy load_options into environment


From: Glenn Washburn
Subject: Re: [PATCH] efi: Copy load_options into environment
Date: Thu, 11 Aug 2022 14:34:24 -0500

On Tue, 8 Dec 2020 20:39:50 -0600
Jordan Webb <jordan@getseam.com> wrote:

> When GRUB is loaded as an EFI application, it will copy the EFI
> LoadOptions into an environment variable called "efi_load_options" and
> export it.

I'm not familiar with EFI LoadOptions, however they don't seem to be a
string necessarily as implied below. The UEFI 2.9 spec in section 9.1
describes LoadOptions as:
  A pointer to the image’s binary load options. See the OptionalData
  parameter in the 1 section of the Boot Manager chapter for
  information on the source of the LoadOptions data."

So this does not sound like necessarily a string and its type is a void
pointer. Perhaps there should be a variable "efi_load_options_str" for
the stringified representation done below and the variable
"efi_load_options" variable should be for the raw binary data. I'm not
adamant about this, but I think it makes more sense.

Also, this variable(s) should be documented in docs/grub.texi.

> 
> My use case for this is to pass along arguments generated by the
> Raspberry Pi's internal bootloader from U-Boot to GRUB to a Linux
> kernel, but I think it's something that could be generally useful to
> other people doing interesting things in EFI environments.

Needs SOB (Signed-off-by).

> ---
>  grub-core/kern/efi/init.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index 2c31847bf..29281c3c5 100644
> --- a/grub-core/kern/efi/init.c
> +++ b/grub-core/kern/efi/init.c
> @@ -17,6 +17,7 @@
>   *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <grub/charset.h>
>  #include <grub/efi/efi.h>
>  #include <grub/efi/console.h>
>  #include <grub/efi/disk.h>
> @@ -31,6 +32,10 @@ grub_addr_t grub_modbase;
>  void
>  grub_efi_init (void)
>  {
> +  grub_efi_loaded_image_t *image = NULL;
> +  int len;
> +  char *load_options;
> +
>    grub_modbase = grub_efi_modules_addr ();
>    /* First of all, initialize the console so that GRUB can display
>       messages.  */
> @@ -43,6 +48,14 @@ grub_efi_init (void)
>             0, 0, 0, NULL);
>  
>    grub_efidisk_init ();
> +
> +  /* Copy EFI load options into environment */
> +  image = grub_efi_get_loaded_image (grub_efi_image_handle);

Should only do the following if the above does not error.

> +  len = image->load_options_size / sizeof (grub_efi_char16_t);
> +  load_options = grub_malloc (len * sizeof (char));

Error check that grub_malloc returns a good pointer.

Also, I believe it should be (len + 1) so that the NULL byte written
below is not out of bounds.

> +  *grub_utf16_to_utf8 ((grub_uint8_t *) load_options, image->load_options, 
> len) = '\0';
> +  grub_env_set("efi_load_options", load_options);
> +  grub_env_export("efi_load_options");

Space before '(' for both lines above.

Glenn

>  }
>  
>  void (*grub_efi_net_config) (grub_efi_handle_t hnd,



reply via email to

[Prev in Thread] Current Thread [Next in Thread]