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

[Daniel Kiper]

On Fri, Nov 25, 2022 at 03:37:35PM +0800, Gary Lin via Grub-devel wrote:
> Per "man 5 cpio", the namesize in the cpio header includes the trailing
> NUL byte of the pathname and the pathname is followed by NUL bytes, but
> the current implementation ignores the trailing NUL byte when making
> the newc header. Although make_header() tries to 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.
>
> Assume that a file is created with 'echo -n aaaa > /boot/test12' and
> loaded by grub2:
>
>     linux /boot/vmlinuz
>     initrd newc:test12:/boot/test12 /boot/initrd
>
> The initrd command eventually invoked grub_initrd_load() and sent
> 't''e''s''t''1''2' to make_header() to generate the header:
>
> 00000070  30 37 30 37 30 31 33 30  31 43 41 30 44 45 30 30  |070701301CA0DE00|
> 00000080  30 30 38 31 41 34 30 30  30 30 30 33 45 38 30 30  |0081A4000003E800|
> 00000090  30 30 30 30 36 34 30 30  30 30 30 30 30 31 36 33  |0000640000000163|
> 000000a0  37 36 45 34 35 32 30 30  30 30 30 30 30 34 30 30  |76E4520000000400|
> 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 74 65  |00000600000000te|
>                                                                   ^namesize
> 000000e0  73 74 31 32 61 61 61 61  30 37 30 37 30 31 30 30  |st12aaaa07070100|
>                    ^^ end of the pathname
>
> Since strlen("test12") + sizeof(struct newc_head) is 116 = 29 * 4,
> make_header() didn't pad the pathname, and the file content followed
> "test12" immediately. This violates the cpio format and may trigger such
> error during linux boot:
>
>     Initramfs unpacking failed: ZSTD-compressed data is trunc
>
> To avoid the potential problems, this commit counts the trailing NUL byte
> in when calling make_header() and adjusts the initrd size accordingly.
>
> Now the header becomes
>
> 00000070  30 37 30 37 30 31 33 30  31 43 41 30 44 45 30 30  |070701301CA0DE00|
> 00000080  30 30 38 31 41 34 30 30  30 30 30 33 45 38 30 30  |0081A4000003E800|
> 00000090  30 30 30 30 36 34 30 30  30 30 30 30 30 31 36 33  |0000640000000163|
> 000000a0  37 36 45 34 35 32 30 30  30 30 30 30 30 34 30 30  |76E4520000000400|
> 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 74 65  |00000700000000te|
>                                                                   ^namesize
> 000000e0  73 74 31 32 00 00 00 00  61 61 61 61 30 37 30 37  |st12....aaaa0707|
>                       ^^ end of the pathname
>
> Besides the trailing NUL byte, make_header() pads 3 more NUL bytes, and
> the user can safely read the pathname without a further check.
>
> To conform to the cpio format, the headers for "TRAILER!!!" are also
> adjusted to include the trailing NUL byte, not ignore it.
>
> Signed-off-by: Gary Lin <glin@suse.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Thank you!

Daniel