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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel