grub-devel
[Top][All Lists]
Advanced

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

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


From: Gary Lin
Subject: Re: [PATCH] loader: Ensure the newc pathname is NULL-terminated
Date: Thu, 24 Nov 2022 11:54:28 +0800

On Wed, Nov 23, 2022 at 03:44:54PM +0100, Daniel Kiper wrote:
> 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...
> 
Will fix in v2.

> > 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?
Because I need the extra '\0' for make_header(). For example, if "name"
is "dir123/file", insert_dir() only sends 'd''i''r''1''2''3' to
make_header() to create the header like this:

00000070  30 37 30 37 30 31 32 31  38 33 32 41 37 43 30 30 |07070121832A7C00|
00000080  30 30 34 31 45 44 30 30  30 30 30 33 45 38 30 30 |0041ED000003E800|
00000090  30 30 30 30 36 34 30 30  30 30 30 30 30 33 36 33 |0000640000000363|
000000a0  37 37 32 44 43 38 30 30  30 30 30 30 30 30 30 30 |772DC80000000000|
000000b0  30 30 30 30 30 38 30 30  30 30 30 30 31 33 30 30 |0000080000001300|
000000c0  30 30 30 30 30 30 30 30  30 30 30 30 30 30 30 30 |0000000000000000|
000000d0  30 30 30 30 30 36 30 30  30 30 30 30 30 30 64 69 |00000600000000di|
000000e0  72 31 32 33 30 37 30 37  30 31 31 31 34 44 37 46 |r123070701114D7F|
                   ^
                   end of "dir123"

As you can see, the pathname is ended without '\0', and it violates the
cpio format. If we give make_header() the extra '\0':

00000070  30 37 30 37 30 31 32 31  38 33 32 41 37 43 30 30 |07070121832A7C00|
00000080  30 30 34 31 45 44 30 30  30 30 30 33 45 38 30 30 |0041ED000003E800|
00000090  30 30 30 30 36 34 30 30  30 30 30 30 30 33 36 33 |0000640000000363|
000000a0  37 37 32 44 43 38 30 30  30 30 30 30 30 30 30 30 |772DC80000000000|
000000b0  30 30 30 30 30 38 30 30  30 30 30 30 31 33 30 30 |0000080000001300|
000000c0  30 30 30 30 30 30 30 30  30 30 30 30 30 30 30 30 |0000000000000000|
000000d0  30 30 30 30 30 37 30 30  30 30 30 30 30 30 64 69 |00000700000000di|
000000e0  72 31 32 33 00 00 00 00  30 37 30 37 30 31 31 31 |r123....07070111|

There are 3 more NUL bytes appended, and the user can safely read the
pathname without a further check.

> And if it is needed you have to check for NULL here.
Okay. Will add the check in v2.

> I do not mention that formatting is broken too. You use spaces instead
> of tabs here and below.
>
I thought the indentation is 2-space without tab. Will convert 8 spaces
into one tab.

> >           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.
>
The adjustment is to match the change of "TRAILER!!!" below.
 
> >                     &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);
The "TRAILER!!!" header should count the NUL byte, not ignore it.

Gary Lin

> >       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
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



reply via email to

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