Re: Tag-alignment in multiboot2 image headers

[Daniel Kiper]

Hi all,

On Thu, Mar 09, 2017 at 09:34:34PM +0000, Ahmed, Safayet (GE Global Research, 
US) wrote:
> Thank you all for the fast response.
>
> 8-byte alignment makes sense. I was looking at another implementation of 
> multiboot2:
> https://github.com/todorez/tummiboot
>
> I thought this implementation was using a 2-byte alignment. On reviewing the 
> code, I realized I misread it.
>
> Thank you for the clarification.
>
> Safayet
>
> From: Vladimir 'phcoder' Serbinenko [mailto:address@hidden
> Sent: Thursday, March 09, 2017 1:37 AM
> To: Andrey Borzenkov <address@hidden>; Ahmed, Safayet (GE Global Research, 
> US) <address@hidden>; Daniel Kiper <address@hidden>

Hmmm... It is interesting. I have not received original email...
So, sorry for late reply.

> Cc: address@hidden
> Subject: EXT: Re: Tag-alignment in multiboot2 image headers
>
>
> On Wed, Mar 8, 2017, 22:28 Andrei Borzenkov 
> <address@hidden<mailto:address@hidden>> wrote:
> On Thu, Mar 9, 2017 at 1:17 AM, Ahmed, Safayet (GE Global Research,
> US) <address@hidden<mailto:address@hidden>> wrote:
> > Hello,
> >
> > I'm seeing an inconsistency in the multiboot2 specification and the 
> > implementation of the multiboot2 loader code in GRUB. It may be my 
> > understanding that's incorrect. A clarification would be appreciated.
> >
> > This concerns the alignment requirements for tags in OS image headers. The 
> > specification states 4 bytes, but the code uses 8 bytes.
> >
> > The specification states (Section 3.1.3) that "Tags constitutes a buffer of 
> > structures following each other padded on 'u32' size."
>
> This is ambiguous and needs better wording as well (it is not clear
> whether "padded" here applies to individual tag or all tags block).

We have to fix the spec. I will do that tomorrow.

> > The "for" loop for parsing tags uses the following "increment" statement 
> > (grub/grub_core/loader/multiboot_mbi2.c: line 148):
> >     tag = (struct multiboot_header_tag *) ((grub_uint32_t *) tag + ALIGN_UP 
> > (tag->size, MULTIBOOT_TAG_ALIGN) / 4))
> >
> > The macro MULTIBOOT_TAG_ALIGN is defined in (include/multiboot2.h) as 8. 
> > This alignment value is consistent with the specification for tags in the 
> > multiboot2 information structure, but not for tags in an OS image header.

As I can see GRUB2 code properly enforces alignment for both.
Though spec is really unclear. So, I will fix it.

> Yes, it sure looks wrong. Thanks for making us aware!
>
> @Vladimir, @Daniel - I think this is 2.02 material, we do not want
> release with such inconsistency. The question is what needs fixing
> though - about half of all tags are not multiple of 8 bytes, so I
> expect people to hit it in real life. What is current implementation
> in MB2 compliant kernels?
> The intention was 8, so that we can have u64 naturally aligned in the
> header even on non-x86. I believe all current kernels are compatible
> with grub, so I think we should update the docs. Daniel, your opinion?

I agree. I will do it tomorrow. Though, IMO, it should not delay RC2 cutting
because spec changes go to separate branch.

Daniel