Re: [PATCH] loader: Ensure the newc pathname is NULL-terminated

[Daniel Kiper]

On Wed, Nov 23, 2022 at 02:40:21PM +0800, Gary Lin via Grub-devel wrote:
> Per "man 5 cpio", the namesize in the cpio header includes the trailing
> NULL byte of the pathname and the pathname is followed by NULL bytes, but

s/NULL/NUL/ and below please...

> the current implementation excludes the trailing NULL byte when making
> the newc header plus the pathname. Although make_header() would pad the
> pathname string, the padding won't happen when strlen(name) +
> sizeof(struct newc_head) is a multiple of 4, and the non-NULL-terminated
> pathname may lead to unexpected results.
>
> For example, when loading linux with the following command:
>
>     linux /boot/vmlinuz
>     initrd newc:test12:/boot/test12 /boot/initrd
>
> Since strlen("test12") + sizeof(struct newc_head) is 116 = 29 * 4, there
> is no padding for the pathname, and the following error may show during
> linux boot:
>
>     Initramfs unpacking failed: ZSTD-compressed data is trunc
>
> To avoid the potential problems, this commit includes the NULL byte when
> calling make_header() and adjusts the initrd size accordingly.
>
> Signed-off-by: Gary Lin <glin@suse.com>
> ---
>  grub-core/loader/linux.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/loader/linux.c b/grub-core/loader/linux.c
> index 830360172..2b26d293f 100644
> --- a/grub-core/loader/linux.c
> +++ b/grub-core/loader/linux.c
> @@ -127,12 +127,14 @@ insert_dir (const char *name, struct dir **root,
>         n->name = grub_strndup (cb, ce - cb);
>         if (ptr)
>           {
> +              char *tmp_name = grub_strndup (name, ce - name);

Why is this needed? And if it is needed you have to check for NULL here.
I do not mention that formatting is broken too. You use spaces instead
of tabs here and below.

>             grub_dprintf ("linux", "Creating directory %s, %s\n", name, ce);
> -           ptr = make_header (ptr, name, ce - name,
> +           ptr = make_header (ptr, tmp_name, ce - name + 1,
>                                040777, 0);
> +              grub_free (tmp_name);
>           }
>         if (grub_add (*size,
> -                     ALIGN_UP ((ce - (char *) name)
> +                     ALIGN_UP ((ce - (char *) name + 1)
>                                 + sizeof (struct newc_head), 4),
>                       size))
>           {
> @@ -191,7 +193,7 @@ grub_initrd_init (int argc, char *argv[],
>                 grub_initrd_close (initrd_ctx);
>                 return grub_errno;
>               }
> -           name_len = grub_strlen (initrd_ctx->components[i].newc_name);
> +           name_len = grub_strlen (initrd_ctx->components[i].newc_name) + 1;
>             if (grub_add (initrd_ctx->size,
>                           ALIGN_UP (sizeof (struct newc_head) + name_len, 4),
>                           &initrd_ctx->size) ||
> @@ -205,7 +207,7 @@ grub_initrd_init (int argc, char *argv[],
>       {
>         if (grub_add (initrd_ctx->size,
>                       ALIGN_UP (sizeof (struct newc_head)
> -                               + sizeof ("TRAILER!!!") - 1, 4),
> +                               + sizeof ("TRAILER!!!"), 4),

This change looks strange and begs for explanation in the patch.

>                       &initrd_ctx->size))
>           goto overflow;
>         free_dir (root);
> @@ -233,7 +235,7 @@ grub_initrd_init (int argc, char *argv[],
>        initrd_ctx->size = ALIGN_UP (initrd_ctx->size, 4);
>        if (grub_add (initrd_ctx->size,
>                   ALIGN_UP (sizeof (struct newc_head)
> -                           + sizeof ("TRAILER!!!") - 1, 4),
> +                           + sizeof ("TRAILER!!!"), 4),

Ditto and bellow...

>                   &initrd_ctx->size))
>       goto overflow;
>        free_dir (root);
> @@ -304,7 +306,7 @@ grub_initrd_load (struct grub_linux_initrd_context 
> *initrd_ctx,
>       }
>        else if (newc)
>       {
> -       ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!") - 1,
> +       ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!"),
>                            0, 0);
>         free_dir (root);
>         root = 0;
> @@ -327,7 +329,7 @@ grub_initrd_load (struct grub_linux_initrd_context 
> *initrd_ctx,
>      {
>        grub_memset (ptr, 0, ALIGN_UP_OVERHEAD (cursize, 4));
>        ptr += ALIGN_UP_OVERHEAD (cursize, 4);
> -      ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!") - 1, 0, 0);
> +      ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!"), 0, 0);

Daniel