Re: Multiboot ELF Loader - Alignment

[Daniel Kiper]

On Fri, Oct 29, 2021 at 08:16:36PM +0100, Chris Plant wrote:
> Daniel,
>
> I'm back home now and I have access to my PC again, so I can answer
> more completely.
>
> On Tue, 2021-10-26 at 14:51 +0100, Chris Plant wrote:
> > Thanks for the detailed reply, I think I’ve got to the bottom of this
> > now, I realised what I missed just after I sent this email.
> >
> > > On 26 Oct 2021, at 13:35, Daniel Kiper <dkiper@net-space.pl> wrote:
> > >
> > > Hey,
> > >
> > > > On Thu, Oct 21, 2021 at 08:46:06PM +0100, Chris Plant wrote:
> > > > Hello,
> > > >
> > > > I'm continuing to play with Multiboot2 on ARM64 on a Raspberry
> > > > Pi, and
> > > > I've run into something which I'm trying to understand.
> > >
> > > Why do you use Multiboot2 on ARM64?
> >
> > I guess the question is why wouldn’t I?  Is the assumption that I
> > should use ARM32 and switch after booting?
> >
> > >
> > > > I have an ELF file for my kernel which has two segments (I have
> > > > no idea
> > > > why rust is giving me two segments, but it is).  If I boot
> > > > directly
> > > > into the ELF file from the Pi firmware it boots fine, but if I
> > > > boot via
> > > > GRUB I have issues with data corruption in .rodata which is in
> > > > the
> > > > second segment.  The first segment (.text) appears to load
> > > > correctly.
> > >
> > > Could you share the output from "readelf -Wa <kernel>"?
> > > Additionally,
> > > how is your Multiboot2 header defined in the kernel?
> >
> > I don’t have this to hand but I will share when I get back.  The
> > multiboot2 header is defined in ASM at the very head of my code.
>
> I've used readelf -la rather than -Wa to avoid including the debugging
> symbols which I don't believe add any value here.
>
> First, the original, which fails to boot correctly:
>
> Elf file type is EXEC (Executable file)
> Entry point 0x800000
> There are 3 program headers, starting at offset 64
>
> Program Headers:
>   Type           Offset             VirtAddr           PhysAddr
>                  FileSiz            MemSiz              Flags  Align
>   LOAD           0x0000000000000200 0x0000000000800000
> 0x0000000000800000
>                  0x0000000000000fd4 0x0000000000000fd4  R E    0x100
>   LOAD           0x00000000000011e0 0x0000000000800fe0
> 0x0000000000800fe0
>                  0x00000000000000e0 0x00000000000000e0  R      0x10
>   GNU_STACK      0x0000000000000000 0x0000000000000000
> 0x0000000000000000
>                  0x0000000000000000 0x0000000000000000  RW     0x0
>
>  Section to Segment mapping:
>   Segment Sections...
>    00     .text
>    01     .rodata
>    02
>
> And the revised, working code, with ld script used to force alignment
> of the second segment to 0x1000 (i.e. page aligned):
>
> Elf file type is EXEC (Executable file)
> Entry point 0x800000
> There are 3 program headers, starting at offset 64
>
> Program Headers:
>   Type           Offset             VirtAddr           PhysAddr
>                  FileSiz            MemSiz              Flags  Align
>   LOAD           0x0000000000000200 0x0000000000800000
> 0x0000000000800000
>                  0x0000000000000fd4 0x0000000000000fd4  R E    0x100
>   LOAD           0x00000000000011e0 0x0000000000801000
> 0x0000000000801000
>                  0x0000000000001000 0x0000000000001000  R      0x10
>   GNU_STACK      0x0000000000000000 0x0000000000000000
> 0x0000000000000000
>                  0x0000000000000000 0x0000000000000000  RW     0x0
>
>  Section to Segment mapping:
>   Segment Sections...
>    00     .text
>    01     .rodata
>    02
> >
> > >
> > > > Digging into this, the ELF headers specify a load base address of
> > > > 0x801060 for the second segment, but GRUB allocates and loads
> > > > this to
> > > > 0x802000.  If I change my linker to align the second segment onto
> > > > 0x1000 everything works fine.
> > >
> > > The Multiboot2 protocol does not tolerate "holes" in the image. You
> > > can
> > > find good explanation what I mean by that here [1].
> >
> > I’ll look into this.
>
> I've read the article and I think my "fixed" linking *is* an image with
> a hole as you describe, since we now ask for loading at 0x800000 - less
> than 0x801000, and then from 0x801000 to whatever.

Yep!

> > > > Is this alignment to 0x1000 a defined behaviour of the GRUB
> > > > allocator
> > > > or Multiboot2?  I'm assuming it's not a problem with the ELF file
> > > > as
> > > > it's generated by usual means and runs fine otherwise.
> > >
> > > I think you should take closer look at grub-
> > > core/loader/multiboot_elfxx.c
> > > and especially lines starting from 258. It seems to me something
> > > around
> > > here overwrites part of the image in the memory.
> > >
> >
> > I did this and I now understand what is causing the issue.  The
> > memory allocator in grub allocates pages and hence aligns the memory
> > region to 0x1000.  When the allocator returns it gives the start of
> > the region allocated and the segment is loaded to that address, not
> > the address specified by the elf file.
> >
> > The workaround I’ve used is to align your second segment to 0x1000 or
> > to only use one segment.
> >
> > Should the defined behaviour be to require segments to be aligned on
> > pages, or should grub load as per the elf file?
>
> The issue is in grub-core/loader/multiboot_elfxx.c, line 134:
>
> for (i = 0; i < ehdr->e_phnum; i++)
>
> ...
>
> If the image is not relocatable, grub tries to allocate each segment
> individually (line 149) (if it is relocatable, grub allocates the
> entire requested, continuous block in one allocation, line 112)
>   
> err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
>                             phdr(i)->p_paddr, phdr(i)->p_memsz);
>
> Although we ask for the base address to be the segment load address,
> the relocator allocates pages, and hence the result is page aligned.

I am not fully convinced. Could you check where exactly this happens in
the GRUB memory allocator? Additionally, could you check what happens
if you use grub_relocator_alloc_chunk_align() instead of
grub_relocator_alloc_chunk_addr() here? Of course the arguments for the
grub_relocator_alloc_chunk_align() should in theory lead to the same
allocation which should be done by the grub_relocator_alloc_chunk_addr().

> We then convert this to a virtual address at line 158:
>
>             source = get_virtual_current_address (ch);
>
> We then load the segment to the allocated region from line 149/158 at
> line 167:
>
>       if (grub_file_read (mld->file, (grub_uint8_t *) source +
> load_offset, phdr(i)->p_filesz)
>                 != (grub_ssize_t) phdr(i)->p_filesz)
>
> So there is no check that the allocated region is the same as the load
> address in the linked file, and because we allocate by page, if there
> is > 1 segment, your segments must be page aligned for this code to
> work.

It seems to me we should introduce checks here which compare allocated
address with the requested one. Hmmm...I think the
grub_relocator_alloc_chunk_addr() should fail if it is not able to
fulfill the request. It should not introduce any alignments because caller
asked for exact address. If this does not work in that way it is a bug.

> I'm happy to fix this, mostly by copying the approach used for
> relocatable code, which fits with your comments about there being no
> holes in the image.

I think you should check [1] first. The most interesting thing is the
note at the bottom of this section: This information does not need to be
provided if the kernel image is in ELF format, but it must be provided
if the image is in a.out format or in some other format. When the
address tag is present it must be used in order to load the image,
regardless of whether an ELF header is also present. Compliant boot
loaders must be able to load images that are either in ELF format or
contain the address tag embedded in the Multiboot2 header.

So, my understating is that the ELF header has to be ignored if the address
tag of Multiboot2 header is present. If this is not the case this is the
bug. Though if both are present they values should be aligned properly
to not make any confusion.

I think it would help if you share your Multiboot2 header with us.

Daniel

[1] 
https://www.gnu.org/software/grub/manual/multiboot2/multiboot.html#Address-header-tag