[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Multiboot 2 Header Alignment: implementation contradicts specificati
From: |
Hans Ulrich Niedermann |
Subject: |
Re: Multiboot 2 Header Alignment: implementation contradicts specification |
Date: |
Sat, 23 May 2020 20:33:17 +0200 |
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