Re: Multiboot ELF segment handling patch

[Daniel Kiper]

On Thu, Mar 29, 2018 at 11:20:37AM +0200, Alexander Boettcher wrote:
> Hello,
>
> our project uses various (micro)kernels booted via GRUB2 using the
> Multiboot & Multiboot2 protocol. Some of the kernels have several ELF
> segments which are not necessarily placed close together.
>
> When booting such kernels with GRUB2 in BIOS legacy mode we observed
> that the screen shows colored blinking scrambled output, until the
> graphic drivers takes over. (Looks very fancy, but our customers don't
> like it ;-) ). We could track down the issue and have a fix for GRUB2,

That is sad... ;-)))

> which is attached.
>
> Can you please have a look and check regarding what should/could be
> changed to get it upstream? We did not test the dynamic relocation part,

Sure thing, please take a look below...

> since we have no such (kernel) setup. Thanks in advance.

You can use Xen (git://xenbits.xen.org/xen.git) for tests.
Just compile hypervisor without any tools and use xen.gz.
It produces nice output and you can see it is relocated or not.
Of course you have to use Multiboot2 protocol.

> Regards,
>
> Alex.
>
> --
> Alexander Boettcher
> Genode Labs
>
> http://www.genode-labs.com - http://www.genode.org -
> https://github.com/genodelabs/genode
>
> Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
> Gesch??ftsf??hrer: Dr.-Ing. Norman Feske, Christian Helmuth


> From 605ca41045f97f92fb698ea49d7267e1cb29f40c Mon Sep 17 00:00:00 2001
> From: Alexander Boettcher <address@hidden>
> Date: Tue, 20 Mar 2018 09:21:06 +0100
> Subject: [PATCH] multiboot: use per segment a separate relocator chunk
>
> If the segments are not dense packed, the original code set up a huge
> relocator chunk comprising all segments.
>
> During the final relocation in grub_relocator_prepare_relocs, the chunk
> is moved to its desired place and overrides memory which are actually not
> covered/touched by the specified segments.
>
> The overridden memory may contain reserved memory
> (vga text mode, acpi tables e.g.), which leads to strange boot behaviour.

Well, this should not happen. I think that this is GRUB2 memory
allocator and/or relocator issue. Both should not touch anything
which is not real RAM. If you know how to trigger this could you
dive in it a bit deeper and check why this happens? Otherwise we
will be hit by the issue sooner or later again. Even if this patch
in any form is accepted.

However, this does not mean that I will not take this patch. Though
it requires some tweaking.

First of all, lack of SOB.

> ---
>  grub-core/loader/multiboot_elfxx.c | 59 
> +++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..0d10c64 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -57,7 +57,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>    char *phdr_base;
>    grub_err_t err;
>    grub_relocator_chunk_t ch;
> -  grub_uint32_t load_offset, load_size;
> +  grub_uint32_t load_size;
>    int i;
>    void *source;
>
> @@ -99,29 +99,6 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>
>    load_size = highest_load - mld->link_base_addr;
>
> -  if (mld->relocatable)
> -    {
> -      if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - 
> load_size)
> -     return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or 
> load size");
> -
> -      err = grub_relocator_alloc_chunk_align (GRUB_MULTIBOOT (relocator), 
> &ch,
> -                                           mld->min_addr, mld->max_addr - 
> load_size,
> -                                           load_size, mld->align ? 
> mld->align : 1,
> -                                           mld->preference, 
> mld->avoid_efi_boot_services);
> -    }
> -  else
> -    err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
> -                                        mld->link_base_addr, load_size);
> -
> -  if (err)
> -    {
> -      grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> -      return err;
> -    }
> -
> -  mld->load_base_addr = get_physical_target_address (ch);
> -  source = get_virtual_current_address (ch);
> -
>    grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, 
> load_base_addr=0x%x, "
>               "load_size=0x%x, relocatable=%d\n", mld->link_base_addr,
>               mld->load_base_addr, load_size, mld->relocatable);
> @@ -133,21 +110,44 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
>    /* Load every loadable segment in memory.  */
>    for (i = 0; i < ehdr->e_phnum; i++)
>      {
> -      if (phdr(i)->p_type == PT_LOAD)
> +      if (phdr(i)->p_type != PT_LOAD)
> +        continue;
> +
> +      if (mld->relocatable)
>          {
> +          if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - 
> load_size)
> +            return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address 
> and/or load size");
> +
> +          err = grub_relocator_alloc_chunk_align (GRUB_MULTIBOOT 
> (relocator), &ch,
> +                                                  mld->min_addr, 
> mld->max_addr - load_size,
> +                                                  phdr(i)->p_memsz, 
> mld->align ? mld->align : 1,
> +                                                  mld->preference, 
> mld->avoid_efi_boot_services);

If we have relocatable image with multiple PT_LOAD segments then
this means that all of them will be put in random places. And we
are able to pass only one load_base_addr value to the image (look
at multiboot_tag_load_base_addr in include/multiboot2.h). Hence,
image itself is able to calculate proper addresses only for one
segment. Not good... So, I think that the lowest segment should
be allocated first using grub_relocator_alloc_chunk_align() and
load_offset should be calculated. Then other segments should be
allocated using grub_relocator_alloc_chunk_addr() and loaded using
calculated offset. This way everything should work. Of course this
is not flexible as it should be. However, at first it is sufficient.
If somebody needs more flexible solution then he/she should extend
Multiboot2 protocol. No more no less.

> +        } else {
> +          err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), 
> &ch,
> +                                                 phdr(i)->p_paddr, 
> phdr(i)->p_memsz);
> +        }

Daniel