[PATCH v3 1/6] loader/efi/linux: Remove useless assignment

[PATCH v3 1/6] loader/efi/linux: Remove useless assignment

[Frediano Ziglio via Grub-devel]

If the following allocation fails this would leave "load_options"
NULL while "load_options_size" not valid.
If the allocation succeed "load_options_size" is overwritten.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 grub-core/loader/efi/linux.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
index 78ea07ca8..ba268eccb 100644
--- a/grub-core/loader/efi/linux.c
+++ b/grub-core/loader/efi/linux.c
@@ -223,8 +223,7 @@ 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)
-- 
2.49.0


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

[PATCH v3 2/6] include/grub/charset.h: Update documentation

[Frediano Ziglio via Grub-devel]

(grub_size_t) -1 is never returned, the function always return
a not negative values. This is important for overflows considerations.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 include/grub/charset.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/grub/charset.h b/include/grub/charset.h
index 31a3b52dd..df79aae53 100644
--- a/include/grub/charset.h
+++ b/include/grub/charset.h
@@ -117,7 +117,8 @@ grub_utf8_process (grub_uint8_t c, grub_uint32_t *code, int *count)
 /* Convert a (possibly null-terminated) UTF-8 string of at most SRCSIZE
    bytes (if SRCSIZE is -1, it is ignored) in length to a UTF-16 string.
    Return the number of characters converted. DEST must be able to hold
-   at least DESTSIZE characters. If an invalid sequence is found, return -1.
+   at least DESTSIZE characters. If an invalid sequence is found, it is
+   replaced by a question mark ('?').
    If SRCEND is not NULL, then *SRCEND is set to the next byte after the
    last byte used in SRC.  */
 static inline grub_size_t
-- 
2.49.0


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

[PATCH v3 3/6] loader/efi/linux: Do not pass excessive size for source string

[Frediano Ziglio via Grub-devel]

The size passed to grub_utf8_to_utf16 for the source string is
used as a limit for the string if NUL character is not encountered
however len, which is strlen(src)*2+2 is surely greater than
strlen(src).
Pass (grub_size_t) -1 to consider only NUL terminator.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 grub-core/loader/efi/linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
index ba268eccb..5befce4d7 100644
--- a/grub-core/loader/efi/linux.c
+++ b/grub-core/loader/efi/linux.c
@@ -231,7 +231,7 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
 
   loaded_image->load_options_size =
     2 * grub_utf8_to_utf16 (loaded_image->load_options, len,
-			    (grub_uint8_t *) args, len, NULL);
+			    (grub_uint8_t *) args, (grub_size_t) -1, NULL);
 
   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 v3 3/6] loader/efi/linux: Do not pass excessive size for source string

[Daniel Kiper]

On Wed, Jun 25, 2025 at 07:42:24AM +0100, Frediano Ziglio via Grub-devel wrote:
> The size passed to grub_utf8_to_utf16 for the source string is
> used as a limit for the string if NUL character is not encountered
> however len, which is strlen(src)*2+2 is surely greater than
> strlen(src).
> Pass (grub_size_t) -1 to consider only NUL terminator.

We should not blindly assume the NUL is inserted by the loader.
So, still len should be used as a safety net. Or something else
if you think len is wrong...

Daniel

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

Re: [PATCH v3 3/6] loader/efi/linux: Do not pass excessive size for source string

[Frediano Ziglio via Grub-devel]

On Wed, Jun 25, 2025 at 1:27 PM Daniel Kiper <dkiper@net-space.pl> wrote:
>
> On Wed, Jun 25, 2025 at 07:42:24AM +0100, Frediano Ziglio via Grub-devel wrote:
> > The size passed to grub_utf8_to_utf16 for the source string is
> > used as a limit for the string if NUL character is not encountered
> > however len, which is strlen(src)*2+2 is surely greater than
> > strlen(src).
> > Pass (grub_size_t) -1 to consider only NUL terminator.
>
> We should not blindly assume the NUL is inserted by the loader.

Why blindly?

grub_arch_efi_linux_boot_image is declared as

grub_err_t
grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)

here "args" has no explicit length, meaning usually NUL-termination.
Later:

  grub_dprintf ("linux", "linux command line: '%s'\n", args);

meaning NUL-termination, then

  len = (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);

again, meaning "args" NUL-termination.

> So, still len should be used as a safety net. Or something else
> if you think len is wrong...
>

For grub_utf8_to_utf16 the explanation is not so easy, but I would use
grub_strlen(args) and not "len".

> Daniel

Frediano

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

[PATCH v3 4/6] loader/efi/linux: Use proper type for "len" variable

[Frediano Ziglio via Grub-devel]

Although the length should not exceed 2^31 grub_size_t is more
suitable for that variable. "len" is used to compute the size
of buffers which in C is a size_t, not a int. It is used
for GRUB_EFI_BYTES_TO_PAGES which expects unsigned values.
It is assigned to load_options_size which is unsigned, not signed.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 grub-core/loader/efi/linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
index 5befce4d7..3b5c4ed17 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)
-- 
2.49.0


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

[PATCH v3 5/6] loader/efi/linux: Use sizeof instead of constant

[Frediano Ziglio via Grub-devel]

This is more consistent with the above code using
sizeof (grub_efi_char16_t).

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 grub-core/loader/efi/linux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
index 3b5c4ed17..9df49150e 100644
--- a/grub-core/loader/efi/linux.c
+++ b/grub-core/loader/efi/linux.c
@@ -229,9 +229,9 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
   if (!loaded_image->load_options)
     return grub_errno;
 
-  loaded_image->load_options_size =
-    2 * grub_utf8_to_utf16 (loaded_image->load_options, len,
+  len = grub_utf8_to_utf16 (loaded_image->load_options, len,
 			    (grub_uint8_t *) args, (grub_size_t) -1, NULL);
+  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

[PATCH v3 6/6] 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.

Changes since v2
- split changes;
- correct spacing.
---
 grub-core/loader/efi/linux.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
index 9df49150e..e53593408 100644
--- a/grub-core/loader/efi/linux.c
+++ b/grub-core/loader/efi/linux.c
@@ -231,6 +231,8 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
 
   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);
-- 
2.49.0


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

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

[Daniel Kiper]

In general patches #1, #2, #4 and #5 make sense for me. So, you can add
my RB there...

On Wed, Jun 25, 2025 at 07:42:27AM +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.
>
> Changes since v2
> - split changes;
> - correct spacing.
> ---
>  grub-core/loader/efi/linux.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
> index 9df49150e..e53593408 100644
> --- a/grub-core/loader/efi/linux.c
> +++ b/grub-core/loader/efi/linux.c
> @@ -231,6 +231,8 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
>
>    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;

It seems to me this is due to lack of termination in the earlier GRUB args
copy. So, probably this should be fixed there. grub_strncpy() instead of
grub_memcpy()? If you do that then probably my comment for #3 does not
make a lot of sense.

And patch set misses cover letter...

Daniel

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

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

[Frediano Ziglio via Grub-devel]

On Wed, Jun 25, 2025 at 1:41 PM Daniel Kiper <dkiper@net-space.pl> wrote:
>
> In general patches #1, #2, #4 and #5 make sense for me. So, you can add
> my RB there...
>
> On Wed, Jun 25, 2025 at 07:42:27AM +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.
> >
> > Changes since v2
> > - split changes;
> > - correct spacing.
> > ---
> >  grub-core/loader/efi/linux.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
> > index 9df49150e..e53593408 100644
> > --- a/grub-core/loader/efi/linux.c
> > +++ b/grub-core/loader/efi/linux.c
> > @@ -231,6 +231,8 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
> >
> >    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;
>
> It seems to me this is due to lack of termination in the earlier GRUB args
> copy. So, probably this should be fixed there. grub_strncpy() instead of
> grub_memcpy()? If you do that then probably my comment for #3 does not
> make a lot of sense.
>

I would say the issue is that grub_utf8_to_utf16 is not
NUL-terminating the string. Well, not an issue itself, just that in
this case we want the buffer NUL-terminated.

> And patch set misses cover letter...
>

Doing it.

> Daniel

Frediano

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