[PATCH v2] loader/efi/linux: Correctly terminate LoadOptions field

[PATCH v2] loader/efi/linux: Correctly terminate LoadOptions field

[Frediano Ziglio via Grub-devel]

If a simple string for arguments are passed it should be NUL
terminated. This is true for other code but not for "linux"
command.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since v1
- remove useless assignment.
---
 grub-core/loader/efi/linux.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
index 78ea07ca8..0c14f6d8f 100644
--- a/grub-core/loader/efi/linux.c
+++ b/grub-core/loader/efi/linux.c
@@ -190,7 +190,7 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
   grub_efi_boot_services_t *b;
   grub_efi_status_t status;
   grub_efi_loaded_image_t *loaded_image;
-  int len;
+  grub_size_t len;
 
   mempath = grub_malloc (2 * sizeof (grub_efi_memory_mapped_device_path_t));
   if (!mempath)
@@ -223,16 +223,17 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
       grub_error (GRUB_ERR_BAD_FIRMWARE, "missing loaded_image proto");
       goto unload;
     }
-  loaded_image->load_options_size = len =
-    (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
+  len = (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
   loaded_image->load_options =
     grub_efi_allocate_any_pages (GRUB_EFI_BYTES_TO_PAGES (len));
   if (!loaded_image->load_options)
     return grub_errno;
 
-  loaded_image->load_options_size =
-    2 * grub_utf8_to_utf16 (loaded_image->load_options, len,
-			    (grub_uint8_t *) args, len, NULL);
+  len = grub_utf8_to_utf16 (loaded_image->load_options, len,
+			    (grub_uint8_t *) args, (grub_size_t) -1, NULL);
+  /* NUL terminate */
+  ((grub_efi_char16_t*)loaded_image->load_options)[len++] = 0;
+  loaded_image->load_options_size = len * sizeof (grub_efi_char16_t);
 
   grub_dprintf ("linux", "starting image %p\n", image_handle);
   status = b->start_image (image_handle, 0, NULL);
-- 
2.49.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] loader/efi/linux: Correctly terminate LoadOptions field

[Daniel Kiper]

On Tue, Jun 24, 2025 at 09:01:14AM +0100, Frediano Ziglio via Grub-devel wrote:
> If a simple string for arguments are passed it should be NUL
> terminated. This is true for other code but not for "linux"
> command.
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
> Changes since v1
> - remove useless assignment.
> ---
>  grub-core/loader/efi/linux.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
> index 78ea07ca8..0c14f6d8f 100644
> --- a/grub-core/loader/efi/linux.c
> +++ b/grub-core/loader/efi/linux.c
> @@ -190,7 +190,7 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
>    grub_efi_boot_services_t *b;
>    grub_efi_status_t status;
>    grub_efi_loaded_image_t *loaded_image;
> -  int len;
> +  grub_size_t len;

Is it really need? If yes this change should be explained in
the commit message.

>    mempath = grub_malloc (2 * sizeof (grub_efi_memory_mapped_device_path_t));
>    if (!mempath)
> @@ -223,16 +223,17 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
>        grub_error (GRUB_ERR_BAD_FIRMWARE, "missing loaded_image proto");
>        goto unload;
>      }
> -  loaded_image->load_options_size = len =
> -    (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
> +  len = (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
>    loaded_image->load_options =
>      grub_efi_allocate_any_pages (GRUB_EFI_BYTES_TO_PAGES (len));
>    if (!loaded_image->load_options)
>      return grub_errno;
>
> -  loaded_image->load_options_size =
> -    2 * grub_utf8_to_utf16 (loaded_image->load_options, len,
> -			    (grub_uint8_t *) args, len, NULL);
> +  len = grub_utf8_to_utf16 (loaded_image->load_options, len,
> +			    (grub_uint8_t *) args, (grub_size_t) -1, NULL);
> +  /* NUL terminate */
> +  ((grub_efi_char16_t*)loaded_image->load_options)[len++] = 0;

Incorrectly formatted cast...

((grub_efi_char16_t *) loaded_image->load_options)[len++] = 0;

> +  loaded_image->load_options_size = len * sizeof (grub_efi_char16_t);

In general I am not happy with the code shuffling which you are doing in
this patch. You have two options here: you should explain in the commit
message why the shuffling is needed or split patch into two and do
required shuffling, with proper commit message, in the first one and add
NUL terminator in the second.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] loader/efi/linux: Correctly terminate LoadOptions field

[Frediano Ziglio via Grub-devel]

On Tue, Jun 24, 2025 at 4:48 PM Daniel Kiper <dkiper@net-space.pl> wrote:
>
> On Tue, Jun 24, 2025 at 09:01:14AM +0100, Frediano Ziglio via Grub-devel wrote:
> > If a simple string for arguments are passed it should be NUL
> > terminated. This is true for other code but not for "linux"
> > command.
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> > ---
> > Changes since v1
> > - remove useless assignment.
> > ---
> >  grub-core/loader/efi/linux.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
> > index 78ea07ca8..0c14f6d8f 100644
> > --- a/grub-core/loader/efi/linux.c
> > +++ b/grub-core/loader/efi/linux.c
> > @@ -190,7 +190,7 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
> >    grub_efi_boot_services_t *b;
> >    grub_efi_status_t status;
> >    grub_efi_loaded_image_t *loaded_image;
> > -  int len;
> > +  grub_size_t len;
>
> Is it really need? If yes this change should be explained in
> the commit message.
>

Added a commit explaining why this was changed.

> >    mempath = grub_malloc (2 * sizeof (grub_efi_memory_mapped_device_path_t));
> >    if (!mempath)
> > @@ -223,16 +223,17 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
> >        grub_error (GRUB_ERR_BAD_FIRMWARE, "missing loaded_image proto");
> >        goto unload;
> >      }
> > -  loaded_image->load_options_size = len =
> > -    (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
> > +  len = (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
> >    loaded_image->load_options =
> >      grub_efi_allocate_any_pages (GRUB_EFI_BYTES_TO_PAGES (len));
> >    if (!loaded_image->load_options)
> >      return grub_errno;
> >
> > -  loaded_image->load_options_size =
> > -    2 * grub_utf8_to_utf16 (loaded_image->load_options, len,
> > -                         (grub_uint8_t *) args, len, NULL);
> > +  len = grub_utf8_to_utf16 (loaded_image->load_options, len,
> > +                         (grub_uint8_t *) args, (grub_size_t) -1, NULL);
> > +  /* NUL terminate */
> > +  ((grub_efi_char16_t*)loaded_image->load_options)[len++] = 0;
>
> Incorrectly formatted cast...
>
> ((grub_efi_char16_t *) loaded_image->load_options)[len++] = 0;
>

Done.

> > +  loaded_image->load_options_size = len * sizeof (grub_efi_char16_t);
>
> In general I am not happy with the code shuffling which you are doing in
> this patch. You have two options here: you should explain in the commit
> message why the shuffling is needed or split patch into two and do
> required shuffling, with proper commit message, in the first one and add
> NUL terminator in the second.
>

I split the various changes, see v3 series.

> Daniel

Frediano

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel