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

[Daniel Kiper <dkiper@net-space.pl>]

On Mon, May 14, 2018 at 09:02:00PM +0200, Alexander Boettcher wrote:
> >From 8a0296d040a42950cd64e733f7997203975bc184 Mon Sep 17 00:00:00 2001
> From: Alexander Boettcher <alexander.boettcher@genode-labs.com>
> Date: Fri, 13 Apr 2018 23:37:01 +0200
> Subject: [PATCH] mbi: use per segment a separate relocator chunk
>
> if elf is non relocatable.

s/elf/ELF file/

> 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 overriden memory may contain device memory (vga text mode e.g.), which
> leads to strange boot behaviour.

I assume that a given ELF PHDR address/size does not cover VGA memory or
anything like that, so, I am not sure what exactly overwrites this region.
grub_memset() in current line 161 at some point?

> Signed-off-by: Alexander Boettcher <alexander.boettcher@genode-labs.com>
> ---
>  grub-core/loader/multiboot_elfxx.c | 57 +++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 22 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..539c94a 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;

It seems to me that this change is not needed.
I am thinking about "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;
>
>    /* Calculate lowest and highest load address.  */
>    for (i = 0; i < ehdr->e_phnum; i++)
> @@ -97,10 +98,14 @@ 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, load_base_addr=0x%x, "
> +		"relocatable=%d\n", mld->link_base_addr,
> +		mld->load_base_addr, mld->relocatable);

mld->load_base_addr is always ~0 here, so, it does not make sense
to display it here. Though I think that mld->link_base_addr and
mld->relocatable should be shown here. Maybe other values from mld
which are know here, i.e. align, preference, avoid_efi_boot_services
too...

>    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 +113,19 @@ 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
> -    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);
> +      if (err)
> +        {
> +          grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
> +          return err;
> +        }
>
> -  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);
> +      mld->load_base_addr = get_physical_target_address (ch);
> +      source = get_virtual_current_address (ch);
>
> -  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);
> +      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);
> +    }

I think that this grub_dprintf() should be moved...

>    /* Load every loadable segment in memory.  */
>    for (i = 0; i < ehdr->e_phnum; i++)
> @@ -139,7 +136,23 @@ 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;
> +	  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));
> +
> +	      source = get_virtual_current_address (ch);
> +	    }
>
>  	  if (phdr(i)->p_filesz != 0)
>  	    {

... further below, just behind this loop. And mld->load_base_addr should be
shown here too.

Daniel