Multiboot 2 Header Alignment: implementation contradicts specification

Multiboot 2 Header Alignment: implementation contradicts specification

[Jacob Paul]

The Multiboot2 specification specifies that the Multiboot2 header should
be 8-byte (64-bit) aligned:
 >An OS image must contain an additional header called Multiboot2 header,
 >besides the headers of the format used by the OS image. The Multiboot2
 >header must be contained completely within the first 32768 bytes of the
 >OS image, and must be 64-bit aligned. In general, it should come as
 >early as possible, and may be embedded in the beginning of the text
 >segment after the real executable header.

However, the implementation of find_header() in multiboot_mbi2.c looks
like this:
 >static struct multiboot_header *
 >find_header (grub_properly_aligned_t *buffer, grub_ssize_t len)
 >{
 >  struct multiboot_header *header;
 >  /* Look for the multiboot header in the buffer.  The header should
 >     be at least 12 bytes and aligned on a 4-byte boundary.  */
 >  for (header = (struct multiboot_header *) buffer;
 >       ((char *) header <= (char *) buffer + len - 12);
 >       header = (struct multiboot_header *) ((grub_uint32_t *) header 
+ >MULTIBOOT_HEADER_ALIGN / 4))
 >    {
 >      if (header->magic == MULTIBOOT2_HEADER_MAGIC
 >	  && !(header->magic + header->architecture
 >	       + header->header_length + header->checksum)
 >	  && header->architecture == MULTIBOOT2_ARCHITECTURE_CURRENT)
 >	return header;
 >    }
 >  return NULL;
 >}

There are multiple things that doesn't look right to me.
The comment says that the header should be 4-byte aligned while at the
same time, the actual loop only increments header 2 bytes for every
iteration (MULTIBOOT_HEADER_ALIGN=8).
It seems like this was just copied over from multiboot_mbi.c since they
basically are identical with even the same comment.

Is there a genuine problem here, or am I missing something?
If it actually is just lazy copy-pasting; should it be changed, as some
people might actually rely on grub finding the Multiboot2 header even
though it isn't 8-byte aligned?

Jacob

Re: Multiboot 2 Header Alignment: implementation contradicts specification

[Hans Ulrich Niedermann]

On Sat, 23 May 2020 18:50:09 +0200
Jacob Paul via Grub-devel <grub-devel@gnu.org> wrote:

> The Multiboot2 specification specifies that the Multiboot2 header
> should be 8-byte (64-bit) aligned:
>  >An OS image must contain an additional header called Multiboot2
>  >header, besides the headers of the format used by the OS image. The
>  >Multiboot2 header must be contained completely within the first
>  >32768 bytes of the OS image, and must be 64-bit aligned. In
>  >general, it should come as early as possible, and may be embedded
>  >in the beginning of the text segment after the real executable
>  >header.  
> 
> However, the implementation of find_header() in multiboot_mbi2.c looks
> like this:
>  >static struct multiboot_header *
>  >find_header (grub_properly_aligned_t *buffer, grub_ssize_t len)
>  >{
>  >  struct multiboot_header *header;
>  >  /* Look for the multiboot header in the buffer.  The header should
>  >     be at least 12 bytes and aligned on a 4-byte boundary.  */
>  >  for (header = (struct multiboot_header *) buffer;
>  >       ((char *) header <= (char *) buffer + len - 12);
>  >       header = (struct multiboot_header *) ((grub_uint32_t *)
>  > header   
> + >MULTIBOOT_HEADER_ALIGN / 4))
>  >    {
>  >      if (header->magic == MULTIBOOT2_HEADER_MAGIC
>  >	  && !(header->magic + header->architecture
>  >	       + header->header_length + header->checksum)
>  >	  && header->architecture ==
>  >	MULTIBOOT2_ARCHITECTURE_CURRENT) return header;
>  >    }
>  >  return NULL;
>  >}  
> 
> There are multiple things that doesn't look right to me.
> The comment says that the header should be 4-byte aligned while at the
> same time,

The comment is valid for MB1, but not for MB2. Both regarding the
alignment and regarding the size. And regarding the size, this actually
means there is a bug in the code here: An MB2 header is at least 16
bytes for the header magic plus at least 8 bytes for the mb2 header
termination tag. That adds up to 24 bytes for MB2, not 12 bytes as it
did for MB1.

> the actual loop only increments header 2 bytes for every
> iteration (MULTIBOOT_HEADER_ALIGN=8).

Where does it increment by 2 bytes for every iteration? I see it
increment by 2 grub_uint32_t per iteration, i.e. by 8 bytes.

> It seems like this was just copied over from multiboot_mbi.c since
> they basically are identical with even the same comment.

I agree that this appears to just be a copied-the-code issue. However,
as MULTIBOOT_HEADER_ALIGN has changed from 4 to 8, this works. Or
rather, as MULTIBOOT_HEADER_ALIGN in multiboot2.h is 8 in contrast to
MULTIBOOT_HEADER_ALIGN in multiboot.h being 4.

> Is there a genuine problem here, or am I missing something?

I fear you have missed that bytes and grub_uint32_t array elements are
not the same size.

> If it actually is just lazy copy-pasting; should it be changed, as
> some people might actually rely on grub finding the Multiboot2 header
> even though it isn't 8-byte aligned?

MB2 spec says it must be 8-byte aligned, so if anybody relies on an MB2
header being found elsewhere completely contrary to the MB2 spec, that
is their problem.

Uli

Re: Multiboot 2 Header Alignment: implementation contradicts specification

[Jacob Paul]

On 2020-05-23 20:33, Hans Ulrich Niedermann wrote:
> The comment is valid for MB1, but not for MB2. Both regarding the
> alignment and regarding the size. And regarding the size, this actually
> means there is a bug in the code here: An MB2 header is at least 16
> bytes for the header magic plus at least 8 bytes for the mb2 header
> termination tag. That adds up to 24 bytes for MB2, not 12 bytes as it
> did for MB1.

I see, so there is at least that. Might be a good idea to fix it.

> I fear you have missed that bytes and grub_uint32_t array elements are
> not the same size.

I unfortunately did not notice the (grub_uint32_t *), I apologize for 
assuming that it increments by 2 bytes and not u32's. Wouldn't it be a 
lot cleaner if it just incremented it in bytes instead, since you could 
then also remove the division by 4 and the cast? It wouldn't make any 
difference runtime wise, just a bit easier to read.

Jacob

Re: Multiboot 2 Header Alignment: implementation contradicts specification

[Hans Ulrich Niedermann]

On Sat, 23 May 2020 21:24:32 +0200
Jacob Paul via Grub-devel <grub-devel@gnu.org> wrote:

> On 2020-05-23 20:33, Hans Ulrich Niedermann wrote:
> > The comment is valid for MB1, but not for MB2. Both regarding the
> > alignment and regarding the size. And regarding the size, this
> > actually means there is a bug in the code here: An MB2 header is at
> > least 16 bytes for the header magic plus at least 8 bytes for the
> > mb2 header termination tag. That adds up to 24 bytes for MB2, not
> > 12 bytes as it did for MB1.  
> 
> I see, so there is at least that. Might be a good idea to fix it.

See separate patch:

Subject: [PATCH] mb2 loader: Fix header size, alignment for Multiboot 2

> > I fear you have missed that bytes and grub_uint32_t array elements
> > are not the same size.  
> 
> I unfortunately did not notice the (grub_uint32_t *), I apologize for 
> assuming that it increments by 2 bytes and not u32's. Wouldn't it be
> a lot cleaner if it just incremented it in bytes instead, since you
> could then also remove the division by 4 and the cast? It wouldn't
> make any difference runtime wise, just a bit easier to read.

And it could be the same code as is in use elsewhere, namely in the
Multiboot 1 loader.

I have put that into a separate RFC patch:

Subject: [PATCH RFC] mb2 loader: Use the same iteration code for MB2 as for MB1