From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1jcYxc-0002sz-Kw for mharc-grub-devel@gnu.org; Sat, 23 May 2020 14:33:28 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:51660) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jcYxb-0002ss-3R for grub-devel@gnu.org; Sat, 23 May 2020 14:33:27 -0400 Received: from relay01.mx.bawue.net ([193.7.176.67]:45892 helo=smtp.bawue.net) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jcYxZ-000201-Ck for grub-devel@gnu.org; Sat, 23 May 2020 14:33:26 -0400 Received: from n-dimensional.de (p5b17584c.dip0.t-ipconnect.de [91.23.88.76]) (Authenticated sender: pdim@bawue.de) by smtp.bawue.net (Postfix) with ESMTPSA id B4E202013C for ; Sat, 23 May 2020 20:33:18 +0200 (CEST) Date: Sat, 23 May 2020 20:33:17 +0200 From: Hans Ulrich Niedermann To: grub-devel@gnu.org Subject: Re: Multiboot 2 Header Alignment: implementation contradicts specification Message-ID: <20200523203317.491e85f8@n-dimensional.de> In-Reply-To: References: X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Virus-Scanner: SAV Dynamic Interface 2.6.0, Engine: 3.77.1, SAV: 5.75 (6F33267) on relay01.mx.bawue.net using milter-sssp 0.1.0 X-Virus-Scan: Found to be clean. Received-SPF: pass client-ip=193.7.176.67; envelope-from=hun@n-dimensional.de; helo=smtp.bawue.net X-detected-operating-system: by eggs.gnu.org: First seen = 2020/05/23 14:33:22 X-ACL-Warn: Detected OS = Linux 3.11 and newer [fuzzy] X-Spam_score_int: -25 X-Spam_score: -2.6 X-Spam_bar: -- X-Spam_report: (-2.6 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_DNSWL_LOW=-0.7, SPF_PASS=-0.001, URIBL_BLOCKED=0.001 autolearn=_AUTOLEARN X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 23 May 2020 18:33:27 -0000 On Sat, 23 May 2020 18:50:09 +0200 Jacob Paul via Grub-devel 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