* Multiboot 2 Header Alignment: implementation contradicts specification
@ 2020-05-23 16:50 Jacob Paul
2020-05-23 18:33 ` Hans Ulrich Niedermann
0 siblings, 1 reply; 4+ messages in thread
From: Jacob Paul @ 2020-05-23 16:50 UTC (permalink / raw)
To: grub-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Multiboot 2 Header Alignment: implementation contradicts specification
2020-05-23 16:50 Multiboot 2 Header Alignment: implementation contradicts specification Jacob Paul
@ 2020-05-23 18:33 ` Hans Ulrich Niedermann
2020-05-23 19:24 ` Jacob Paul
0 siblings, 1 reply; 4+ messages in thread
From: Hans Ulrich Niedermann @ 2020-05-23 18:33 UTC (permalink / raw)
To: grub-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Multiboot 2 Header Alignment: implementation contradicts specification
2020-05-23 18:33 ` Hans Ulrich Niedermann
@ 2020-05-23 19:24 ` Jacob Paul
2020-05-23 20:26 ` Hans Ulrich Niedermann
0 siblings, 1 reply; 4+ messages in thread
From: Jacob Paul @ 2020-05-23 19:24 UTC (permalink / raw)
To: grub-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Multiboot 2 Header Alignment: implementation contradicts specification
2020-05-23 19:24 ` Jacob Paul
@ 2020-05-23 20:26 ` Hans Ulrich Niedermann
0 siblings, 0 replies; 4+ messages in thread
From: Hans Ulrich Niedermann @ 2020-05-23 20:26 UTC (permalink / raw)
To: grub-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-23 20:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-23 16:50 Multiboot 2 Header Alignment: implementation contradicts specification Jacob Paul
2020-05-23 18:33 ` Hans Ulrich Niedermann
2020-05-23 19:24 ` Jacob Paul
2020-05-23 20:26 ` Hans Ulrich Niedermann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.