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 <address@hidden> 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