Re: [PATCH v2] mbi: use per segment a separate relocator chunk

[Daniel Kiper]

On Tue, Jun 05, 2018 at 10:14:49PM +0200, Alexander Boettcher wrote:
> Instead of setting up a all comprising relocator chunk for all segments,
> use per segment a separate relocator chunk.
>
> If the ELF is non-relocatable, a single relocator chunk will comprise memory
> (between the segments) which gets overridden by the relst() invocation of the
> movers code in grub_relocator16/32/64_boot().
>
> The overridden memory may contain reserved ranges like VGA memory or ACPI
> tables, which leads to strange boot behaviour.
>
> Signed-off-by: Alexander Boettcher <address@hidden>
> ---
>  grub-core/loader/multiboot_elfxx.c | 60 
> +++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c 
> b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..6565b50 100644
> --- a/grub-core/loader/multiboot_elfxx.c
> +++ b/grub-core/loader/multiboot_elfxx.c
> @@ -57,9 +57,9 @@ 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_offset = 0, load_size;
>    int i;
> -  void *source;
> +  void *source = NULL;
>
>    if (ehdr->e_ident[EI_MAG0] != ELFMAG0
>        || ehdr->e_ident[EI_MAG1] != ELFMAG1
> @@ -83,6 +83,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>  #define phdr(i)                      ((Elf_Phdr *) (phdr_base + (i) * 
> ehdr->e_phentsize))
>
>    mld->link_base_addr = ~0;
> +  mld->load_base_addr = ~0;

We do not need it. Please look below.

>    /* Calculate lowest and highest load address.  */
>    for (i = 0; i < ehdr->e_phnum; i++)
> @@ -97,10 +98,15 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>      return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
>  #endif
>
> -  load_size = highest_load - mld->link_base_addr;
> +  grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, relocatable=%d, "
> +             "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n",
> +             mld->link_base_addr, mld->relocatable, (long) mld->align,
> +             mld->preference, mld->avoid_efi_boot_services);

I have just realized that it does not make sense to print align,
preference and avoid_efi_boot_services if mld->relocatable is not
set. Please take a look at grub-core/loader/multiboot_mbi2.c for
more details. Sorry about that. In this case you can probably drop
this grub_dprintf() and...

>    if (mld->relocatable)
>      {
> +      load_size = highest_load - mld->link_base_addr;
> +
>        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");
>
> @@ -108,27 +114,16 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
>                                             mld->min_addr, mld->max_addr - 
> load_size,
>                                             load_size, mld->align ? 
> mld->align : 1,
>                                             mld->preference, 
> mld->avoid_efi_boot_services);
> -    }
> -  else

Please do not drop this else. You can put this here:
  mld->load_base_addr = mld->link_base_addr;

load_base_addr == link_base_addr in non relocatable case.
Am I right?

> -    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);

...you can leave this excluding load_size and...

> +      if (err)
> +        {
> +          grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
> image\n");
> +          return err;
> +        }
>
> -  if (mld->relocatable)
> -    grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, 
> avoid_efi_boot_services=%d\n",
> -               (long) mld->align, mld->preference, 
> mld->avoid_efi_boot_services);

...you can leave this grub_dprintf() and move load_size from above.
You can put it after preference.

> +      mld->load_base_addr = get_physical_target_address (ch);
> +      source = get_virtual_current_address (ch);
> +    }
>
>    /* Load every loadable segment in memory.  */
>    for (i = 0; i < ehdr->e_phnum; i++)
> @@ -139,7 +134,26 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t 
> *mld)
>         grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, 
> memsz=0x%lx, vaddr=0x%lx\n",
>                       i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, 
> (long) phdr(i)->p_vaddr);
>
> -       load_offset = phdr(i)->p_paddr - mld->link_base_addr;
> +       if (mld->relocatable)
> +         load_offset = phdr(i)->p_paddr - mld->link_base_addr;

Please add this here:
  grub_dprintf ("multiboot_loader", "segment %d: load_offset=0x%x\n", i, 
load_offset);

> +       else
> +         {
> +           err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT 
> (relocator), &ch,
> +                                                  phdr(i)->p_paddr, 
> phdr(i)->p_memsz);
> +
> +           if (err)
> +             {
> +               grub_dprintf ("multiboot_loader", "Cannot allocate memory for 
> OS image\n");
> +               return err;
> +             }
> +
> +           mld->load_base_addr = grub_min (mld->load_base_addr, 
> get_physical_target_address (ch));

You can drop this. Please look above.

> +
> +           source = get_virtual_current_address (ch);
> +         }
> +
> +       grub_dprintf ("multiboot_loader", "segment %d: load_base_addr=0x%x, "
> +                     "load_offset=0x%x\n", i, mld->load_base_addr, 
> load_offset);

Please drop this grub_dprintf().

Daniel