On Tue, Jun 24, 2025 at 4:48 PM Daniel Kiper <dki...@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.zig...@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

Reply via email to