Wimboot fails to get initrd data (probably regression)

Wimboot fails to get initrd data (probably regression)

[Bernhard Übelacker]

[-- Attachment #1: Type: text/plain, Size: 4982 bytes --]

Hello,
I built yesterdays git of grub2. I observed that booting
via wimboot 2.2.4 or 2.4.1 is in this combination not
longer possible.

------------

The last lines of the failing boot on screen:
  ...
  Using boot.wim via 0xfca5254 len 0x13fc7f7f
  ...found WIM file boot.wim
  Bad CPIO magic
  FATAL: could not extract initrd files
  Press a key to reboot


The used entry in grub.cfg:
  menuentry " * Win81PEx64 2014-06-14 MistyPE_81_x64 wimboot (usb/pxe)" {
    set isoroot=/boot/windows/win81/2014-06-14_MistyPE_x64/ISO.ROOT
    linux16 /boot/bootloader/wimboot/wimboot-2.4.1/wimboot
    initrd16 \
        newc:bootmgr:${isoroot}/bootmgr \
        newc:bcd:${isoroot}/boot/bcd \
        newc:wgl4_boot.ttf:${isoroot}/boot/fonts/wgl4_boot.ttf \
        newc:boot.sdi:${isoroot}/boot/boot.sdi \
        newc:boot.wim:${isoroot}/sources/boot.wim
  }

------------

A git bisect pointed to following commit:
a8c473288d3f0a5e17a903a5121dea1a695dda3b is the first bad commit                                                                                                                         
commit a8c473288d3f0a5e17a903a5121dea1a695dda3b                                                                                                                                          
Author: Andrei Borzenkov <arvidjaar@gmail.com>
Date:   Thu May 7 20:24:24 2015 +0300

    loader/linux: do not pad initrd with zeroes at the end
    
    Syslinux memdisk is using initrd image and needs to know uncompressed
    size in advance. For gzip uncompressed size is at the end of compressed
    stream. Grub padded each input file to 4 bytes at the end, which means
    syslinux got wrong size.
    
    Linux initramfs loader apparently does not care about trailing alignment.
    So change code to align beginning of each file instead which atomatically
    gives us the correct size for single file.
    
    Reported-By: David Shaw <dshaw@jabberwocky.com>

:040000 040000 f999167e8f6744c6be8abb2df5490617d2d6a5aa c7631f7f7683e9aaa31736f095f8234935727483 M      grub-core

([1] and [2] for the discussions about this change.)

------------

I tried to have a look at the data wimboot is complaining and
found that the five files were extracted successfully but the
trailing element ("TRAILER!!!") was not recognized, because the
data pointer is set one byte too far. Therefore the "magic" does
not match.
Wimboot expects the next "file" to start at a aligned address, even
if last file size was not even.

This is probably because my boot.wim has not a even size.

When cpio_len is not aligned, like in following patch to wimboot,
then the boot can continue successful.


    diff --git a/src/cpio.c b/src/cpio.c
    index affb1d8..a0a1e87 100644
    --- a/src/cpio.c
    +++ b/src/cpio.c
    @@ -112,8 +112,6 @@ int cpio_extract ( void *data, size_t len,
                              file_name_len ) );
            file_len = cpio_value ( cpio->c_filesize );
            cpio_len = ( file_data + file_len - data );
    -		if ( cpio_len < len )
    -			cpio_len = cpio_align ( cpio_len );
            if ( cpio_len > len ) {
                DBG ( "Truncated CPIO file\n" );
                return -1;

-----------

I am not certain if this is a regression with grub or if it
revealed a bug within wimboot.

Therefore I tried to reorder the initrd files and switched
boot.sdi and boot.wim and then it booted also successfully
with unmodified wimboot.
Then this would just be a problem with the trailer entry
and a regression in grub?

I tried therefore to put the "trailer" again to an aligned address.
This makes the boot succeed again (Also attached as patch to current grub git):


    @@ -213,6 +213,7 @@ grub_initrd_init (int argc, char *argv[],
     
       if (newc)
         {
    +      initrd_ctx->size = ALIGN_UP (initrd_ctx->size, 4);
           initrd_ctx->size += ALIGN_UP (sizeof (struct newc_head)
                                        + sizeof ("TRAILER!!!") - 1, 4);
           free_dir (root);
    @@ -290,7 +291,11 @@ grub_initrd_load (struct grub_linux_initrd_context *initrd_ctx,
           ptr += cursize;
         }
       if (newc)
    -    ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!") - 1, 0, 0);
    +    {
    +      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);
    +    }
       free_dir (root);
       root = 0;
       return GRUB_ERR_NONE;

----------

If the documentation in [3] is still valid, it looks to me like
the cpio_trailer really has to be aligned:

    cpio_trailer := ALGN(4) + cpio_header + "TRAILER!!!\0" + ALGN(4)

What do you think?


Kind regards,
Bernhard


[1] http://www.syslinux.org/archives/2015-April/thread.html#23396
[2] http://lists.gnu.org/archive/html/grub-devel/2015-04/threads.html#00095
[3] https://www.kernel.org/doc/Documentation/early-userspace/buffer-format.txt

[-- Attachment #2: 0001-loader-linux-Make-trailer-initrd-entry-aligned-again.patch --]
[-- Type: text/x-patch, Size: 1569 bytes --]

From 6feb332935bb379d7cfce6dfaedbc86cd31cd936 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bernhard=20=C3=9Cbelacker?= <bernhardu@vr-web.de>
Date: Mon, 20 Jul 2015 14:06:45 +0200
Subject: [PATCH] loader/linux: Make trailer initrd entry aligned again.

Regression from commit:
  loader/linux: do not pad initrd with zeroes at the end
  a8c473288d3f0a5e17a903a5121dea1a695dda3b

Wimboot fails since the change above because it expects the "trailer"
initrd element on an aligned address.
This issue shows only when newc_name is used and the last initrd
entry has a not aligned size.
---
 grub-core/loader/linux.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/grub-core/loader/linux.c b/grub-core/loader/linux.c
index d2cd591..be6fa0f 100644
--- a/grub-core/loader/linux.c
+++ b/grub-core/loader/linux.c
@@ -213,6 +213,7 @@ grub_initrd_init (int argc, char *argv[],
 
   if (newc)
     {
+      initrd_ctx->size = ALIGN_UP (initrd_ctx->size, 4);
       initrd_ctx->size += ALIGN_UP (sizeof (struct newc_head)
 				    + sizeof ("TRAILER!!!") - 1, 4);
       free_dir (root);
@@ -290,7 +291,11 @@ grub_initrd_load (struct grub_linux_initrd_context *initrd_ctx,
       ptr += cursize;
     }
   if (newc)
-    ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!") - 1, 0, 0);
+    {
+      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);
+    }
   free_dir (root);
   root = 0;
   return GRUB_ERR_NONE;
-- 
2.1.4

Re: Wimboot fails to get initrd data (probably regression)

[Andrei Borzenkov]

On Mon, Jul 20, 2015 at 3:38 PM, Bernhard Übelacker <bernhardu@vr-web.de> wrote:
>
>
>     @@ -213,6 +213,7 @@ grub_initrd_init (int argc, char *argv[],
>
>        if (newc)
>          {
>     +      initrd_ctx->size = ALIGN_UP (initrd_ctx->size, 4);
>            initrd_ctx->size += ALIGN_UP (sizeof (struct newc_head)
>                                         + sizeof ("TRAILER!!!") - 1, 4);
>            free_dir (root);


But code already does exactly the same.

      initrd_ctx->size = ALIGN_UP (initrd_ctx->size, 4);
...
      else if (newc)
{
 initrd_ctx->size += ALIGN_UP (sizeof (struct newc_head)
+ sizeof ("TRAILER!!!") - 1, 4);


>     @@ -290,7 +291,11 @@ grub_initrd_load (struct grub_linux_initrd_context *initrd_ctx,
>            ptr += cursize;
>          }
>        if (newc)
>     -    ptr = make_header (ptr, "TRAILER!!!", sizeof ("TRAILER!!!") - 1, 0, 0);
>     +    {
>     +      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);
>     +    }
>        free_dir (root);
>        root = 0;
>        return GRUB_ERR_NONE;
>

Ditto. Could you make your failing initrd available?

Re: Wimboot fails to get initrd data (probably regression)

[Andrei Borzenkov]

On Mon, Jul 20, 2015 at 4:47 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>
> But code already does exactly the same.
>

Ignore this. I see. Thank you!

Re: Wimboot fails to get initrd data (probably regression)

[Andrei Borzenkov]

В Mon, 20 Jul 2015 14:38:39 +0200
Bernhard Übelacker <bernhardu@vr-web.de> пишет:

> This makes the boot succeed again (Also attached as patch to current grub git):
> 

Applied.