Multiboot ELF segment handling patch

Multiboot ELF segment handling patch

[Alexander Boettcher]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

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,
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,
since we have no such (kernel) setup. Thanks in advance.

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-multiboot-use-per-segment-a-separate-relocator-chunk.patch --]
[-- Type: text/x-patch; name="0001-multiboot-use-per-segment-a-separate-relocator-chunk.patch", Size: 4994 bytes --]

From 605ca41045f97f92fb698ea49d7267e1cb29f40c Mon Sep 17 00:00:00 2001
From: Alexander Boettcher <alexander.boettcher@genode-labs.com>
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.
---
 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);
+        } 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;
+        }
+
+      if (mld->load_base_addr > get_physical_target_address (ch))
+        mld->load_base_addr = get_physical_target_address (ch);
+
+      source = get_virtual_current_address (ch);
 
 	  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 (phdr(i)->p_filesz != 0)
 	    {
 	      if (grub_file_seek (mld->file, (grub_off_t) phdr(i)->p_offset)
 		  == (grub_off_t) -1)
 		return grub_errno;
 
-	      if (grub_file_read (mld->file, (grub_uint8_t *) source + load_offset, phdr(i)->p_filesz)
+	      if (grub_file_read (mld->file, (grub_uint8_t *) source, phdr(i)->p_filesz)
 		  != (grub_ssize_t) phdr(i)->p_filesz)
 		{
 		  if (!grub_errno)
@@ -158,9 +158,8 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
 	    }
 
           if (phdr(i)->p_filesz < phdr(i)->p_memsz)
-            grub_memset ((grub_uint8_t *) source + load_offset + phdr(i)->p_filesz, 0,
+            grub_memset ((grub_uint8_t *) source + phdr(i)->p_filesz, 0,
 			 phdr(i)->p_memsz - phdr(i)->p_filesz);
-        }
     }
 
   for (i = 0; i < ehdr->e_phnum; i++)
-- 
2.7.4

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 <alexander.boettcher@genode-labs.com>
> 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

Re: Multiboot ELF segment handling patch

[Alexander Boettcher]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

Hello,

On 06.04.2018 14:28, Daniel Kiper wrote:
> On Thu, Mar 29, 2018 at 11:20:37AM +0200, Alexander Boettcher wrote:
>
>> 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.

Thanks, I managed to setup it. Based on your comments and on the Xen
test case I had to re-work the patch, so that it now works for
relocation and non-relocation kernels.

Please review again.
> However, this does not mean that I will not take this patch. Though
> it requires some tweaking.
> 
> First of all, lack of SOB.

What is a SOB ?

Cheers,

-- 
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-mbi-use-per-segment-a-separate-relocator-chunk.patch --]
[-- Type: text/x-patch; name="0001-mbi-use-per-segment-a-separate-relocator-chunk.patch", Size: 4755 bytes --]

From c37727840be8aeb81ea4bbc95ac09a1b1e3d3658 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.

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.
---
 grub-core/loader/multiboot_elfxx.c | 56 ++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 67daf59..d936223 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;
 
   /* Calculate lowest and highest load address.  */
   for (i = 0; i < ehdr->e_phnum; i++)
@@ -108,27 +109,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;
-    }
+      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);
+      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);
-
-  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);
+    }
 
   /* Load every loadable segment in memory.  */
   for (i = 0; i < ehdr->e_phnum; i++)
@@ -139,7 +132,24 @@ 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;
+		}
+
+	      if (mld->load_base_addr > get_physical_target_address (ch))
+	        mld->load_base_addr = get_physical_target_address (ch);
+
+	      source = get_virtual_current_address (ch);
+	    }
 
 	  if (phdr(i)->p_filesz != 0)
 	    {
@@ -163,6 +173,10 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
         }
     }
 
+  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);
+
   for (i = 0; i < ehdr->e_phnum; i++)
     if (phdr(i)->p_vaddr <= ehdr->e_entry
 	&& phdr(i)->p_vaddr + phdr(i)->p_memsz > ehdr->e_entry)
-- 
2.7.4

Re: Multiboot ELF segment handling patch

[Daniel Kiper]

Hi Alexander,

On Sat, Apr 14, 2018 at 12:07:29AM +0200, Alexander Boettcher wrote:
> Hello,
>
> On 06.04.2018 14:28, Daniel Kiper wrote:
> > On Thu, Mar 29, 2018 at 11:20:37AM +0200, Alexander Boettcher wrote:
> >
> >> 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.
>
> Thanks, I managed to setup it. Based on your comments and on the Xen
> test case I had to re-work the patch, so that it now works for
> relocation and non-relocation kernels.
>
> Please review again.
> > However, this does not mean that I will not take this patch. Though
> > it requires some tweaking.
> >
> > First of all, lack of SOB.
>
> What is a SOB ?

Signed-off-by: Alexander Boettcher <alexander.boettcher@genode-labs.com>

Next time please use git format-patch/send-email to send the patches.

> From c37727840be8aeb81ea4bbc95ac09a1b1e3d3658 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.
>
> 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.

Have you been able to take a look at memory allocator/relocator code why
this happened?

> ---
>  grub-core/loader/multiboot_elfxx.c | 56 ++++++++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..d936223 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;
>
>    /* Calculate lowest and highest load address.  */
>    for (i = 0; i < ehdr->e_phnum; i++)
> @@ -108,27 +109,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;
> -    }
> +      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);
> +      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);

I would not drop it completely from ~here. I think that you can display
link_base_addr and relocatable just before current line 102. And you can
put "load_size = highest_load - mld->link_base_addr;" just before current
line 104. Additionally, load_size does not have a real meaning for non
relocatable images right now. Hence, I think that it should not be displayed
for them. Especially if there are more than one PT_LOAD segment.

> -  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);
> +    }
>
>    /* Load every loadable segment in memory.  */
>    for (i = 0; i < ehdr->e_phnum; i++)
> @@ -139,7 +132,24 @@ 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;
> +		}
> +
> +	      if (mld->load_base_addr > get_physical_target_address (ch))
> +	        mld->load_base_addr = get_physical_target_address (ch);

              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)
>  	    {
> @@ -163,6 +173,10 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
>          }
>      }
>
> +  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,

Well load_size is not meaningful for non relocatable images. Hmmm...
You can make it more meaningful by summing up all phdr(i)->p_memsz.

Anyway, patch looks much better right now.

Daniel

Re: Multiboot ELF segment handling patch

[Konrad Rzeszutek Wilk]

On April 17, 2018 3:40:11 PM EDT, Daniel Kiper <dkiper@net-space.pl> wrote:
>Hi Alexander,
>
>On Sat, Apr 14, 2018 at 12:07:29AM +0200, Alexander Boettcher wrote:
>> Hello,
>>
>> On 06.04.2018 14:28, Daniel Kiper wrote:
>> > On Thu, Mar 29, 2018 at 11:20:37AM +0200, Alexander Boettcher
>wrote:
>> >
>> >> 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.
>>
>> Thanks, I managed to setup it. Based on your comments and on the Xen
>> test case I had to re-work the patch, so that it now works for
>> relocation and non-relocation kernels.
>>
>> Please review again.
>> > However, this does not mean that I will not take this patch. Though
>> > it requires some tweaking.
>> >
>> > First of all, lack of SOB.
>>
>> What is a SOB ?
>
>Signed-off-by: Alexander Boettcher
><alexander.boettcher@genode-labs.com>

There is more to it then that. Doing an SoB means you abide by the developer certificate.

See https://developercertificate.org/
>
>Next time please use git format-patch/send-email to send the patches.
>
>> From c37727840be8aeb81ea4bbc95ac09a1b1e3d3658 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.
>>
>> 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.
>
>Have you been able to take a look at memory allocator/relocator code
>why
>this happened?
>
>> ---
>>  grub-core/loader/multiboot_elfxx.c | 56
>++++++++++++++++++++++++--------------
>>  1 file changed, 35 insertions(+), 21 deletions(-)
>>
>> diff --git a/grub-core/loader/multiboot_elfxx.c
>b/grub-core/loader/multiboot_elfxx.c
>> index 67daf59..d936223 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;
>>
>>    /* Calculate lowest and highest load address.  */
>>    for (i = 0; i < ehdr->e_phnum; i++)
>> @@ -108,27 +109,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;
>> -    }
>> +      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);
>> +      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);
>
>I would not drop it completely from ~here. I think that you can display
>link_base_addr and relocatable just before current line 102. And you
>can
>put "load_size = highest_load - mld->link_base_addr;" just before
>current
>line 104. Additionally, load_size does not have a real meaning for non
>relocatable images right now. Hence, I think that it should not be
>displayed
>for them. Especially if there are more than one PT_LOAD segment.
>
>> -  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);
>> +    }
>>
>>    /* Load every loadable segment in memory.  */
>>    for (i = 0; i < ehdr->e_phnum; i++)
>> @@ -139,7 +132,24 @@ 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;
>> +		}
>> +
>> +	      if (mld->load_base_addr > get_physical_target_address (ch))
>> +	        mld->load_base_addr = get_physical_target_address (ch);
>
>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)
>>  	    {
>> @@ -163,6 +173,10 @@ CONCAT(grub_multiboot_load_elf, XX)
>(mbi_load_data_t *mld)
>>          }
>>      }
>>
>> +  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,
>
>Well load_size is not meaningful for non relocatable images. Hmmm...
>You can make it more meaningful by summing up all phdr(i)->p_memsz.
>
>Anyway, patch looks much better right now.
>
>Daniel
>
>_______________________________________________
>Grub-devel mailing list
>Grub-devel@gnu.org
>https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Multiboot ELF segment handling patch

[Alexander Boettcher]

Hello,

On 17.04.2018 21:40, Daniel Kiper wrote
>> The overriden memory may contain device memory (vga text mode e.g.), which
>> leads to strange boot behaviour.
> 
> Have you been able to take a look at memory allocator/relocator code why
> this happened?

for me it looks like, that either already
grub_relocator_alloc_chunk_addr or at latest
grub_relocator_prepare_relocs should bail out with an error, if the
memory range of the VGA memory (0xa0000 ++) is covered.

What I don't know, respectively didn't found in the code, how and where
this information about this VGA text memory area is excluded/reserved
from the allocator/reallocator. Maybe you can give me a hint ?

Cheers,

-- 
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth

Re: Multiboot ELF segment handling patch

[Daniel Kiper]

On Mon, Apr 23, 2018 at 08:26:34PM +0200, Alexander Boettcher wrote:
> Hello,
>
> On 17.04.2018 21:40, Daniel Kiper wrote
> >> The overriden memory may contain device memory (vga text mode e.g.), which
> >> leads to strange boot behaviour.
> >
> > Have you been able to take a look at memory allocator/relocator code why
> > this happened?
>
> for me it looks like, that either already
> grub_relocator_alloc_chunk_addr or at latest
> grub_relocator_prepare_relocs should bail out with an error, if the
> memory range of the VGA memory (0xa0000 ++) is covered.

Exactly!

> What I don't know, respectively didn't found in the code, how and where
> this information about this VGA text memory area is excluded/reserved
> from the allocator/reallocator. Maybe you can give me a hint ?

I have skimmed through the code and I think that you should take
a look at grub-core/kern/i386/pc/init.c, grub-core/kern/mm.c and
grub-core/lib/relocator.c. At first sight it looks that
grub-core/kern/i386/pc/init.c:grub_machine_init() is the
most interesting thing.

I hope that helps. If you have further questions drop me a line.

Thank you for taking a stab at this.

Daniel

Re: Multiboot ELF segment handling patch

[Alexander Boettcher]

On 24.04.2018 12:42, Daniel Kiper wrote:
> On Mon, Apr 23, 2018 at 08:26:34PM +0200, Alexander Boettcher wrote:
>> On 17.04.2018 21:40, Daniel Kiper wrote
>>>> The overriden memory may contain device memory (vga text mode e.g.), which
>>>> leads to strange boot behaviour.
>>>
>>> Have you been able to take a look at memory allocator/relocator code why
>>> this happened?
>>
>> for me it looks like, that either already
>> grub_relocator_alloc_chunk_addr or at latest
>> grub_relocator_prepare_relocs should bail out with an error, if the
>> memory range of the VGA memory (0xa0000 ++) is covered.
> 
> Exactly!
> 
>> What I don't know, respectively didn't found in the code, how and where
>> this information about this VGA text memory area is excluded/reserved
>> from the allocator/reallocator. Maybe you can give me a hint ?
> 
> I have skimmed through the code and I think that you should take
> a look at grub-core/kern/i386/pc/init.c, grub-core/kern/mm.c and
> grub-core/lib/relocator.c. At first sight it looks that
> grub-core/kern/i386/pc/init.c:grub_machine_init() is the
> most interesting thing.
> 
> I hope that helps. If you have further questions drop me a line.
> 
> Thank you for taking a stab at this.

The whole lower 1M physical memory is reserved and not available for use
(done by grub-core/kern/i386/pc/init.c), but effectively ignored by
grub_relocator_alloc_chunk_addr for the non relocatable case during
multiboot segment load.

The adjust_limits call in grub_relocator_alloc_chunk_addr mainly sets
the min_addr to 0 and max_addr to end of physical memory. So
grub_relocator_alloc_chunk_addr will ever find a piece of (higher)
memory, mainly ignoring the target address (which is reserved, e.g.
below 1M).

The showcase code snippet below catches the case now. I don't know
whether it is correct nor do I like it ...

What do you think ?


Alex


--- a/grub-core/lib/relocator.c
+++ b/grub-core/lib/relocator.c
@@ -1226,7 +1237,7 @@ adjust_limits (struct grub_relocator *rel,
 grub_err_t
 grub_relocator_alloc_chunk_addr (struct grub_relocator *rel,
 				 grub_relocator_chunk_t *out,
-				 grub_phys_addr_t target, grub_size_t size)
+				 grub_phys_addr_t target, grub_size_t size, int relocatable)
 {
   struct grub_relocator_chunk *chunk;
   grub_phys_addr_t min_addr = 0, max_addr;
@@ -1250,6 +1263,10 @@ grub_relocator_alloc_chunk_addr (struct
grub_relocator *rel,
 		(unsigned long long) min_addr, (unsigned long long) max_addr,
 		(unsigned long long) target);

+  if (!relocatable &&
+      !malloc_in_range (rel, target, target + size, 1, size, chunk, 0, 1))
+    return grub_error (GRUB_ERR_BUG, "target memory not available");
+
   do
     {
       /* A trick to improve Linux allocation.  */

-- 
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth

Re: Multiboot ELF segment handling patch

[Daniel Kiper]

On Thu, Apr 26, 2018 at 11:55:22PM +0200, Alexander Boettcher wrote:
> On 24.04.2018 12:42, Daniel Kiper wrote:
> > On Mon, Apr 23, 2018 at 08:26:34PM +0200, Alexander Boettcher wrote:
> >> On 17.04.2018 21:40, Daniel Kiper wrote
> >>>> The overriden memory may contain device memory (vga text mode e.g.), which
> >>>> leads to strange boot behaviour.
> >>>
> >>> Have you been able to take a look at memory allocator/relocator code why
> >>> this happened?
> >>
> >> for me it looks like, that either already
> >> grub_relocator_alloc_chunk_addr or at latest
> >> grub_relocator_prepare_relocs should bail out with an error, if the
> >> memory range of the VGA memory (0xa0000 ++) is covered.
> >
> > Exactly!
> >
> >> What I don't know, respectively didn't found in the code, how and where
> >> this information about this VGA text memory area is excluded/reserved
> >> from the allocator/reallocator. Maybe you can give me a hint ?
> >
> > I have skimmed through the code and I think that you should take
> > a look at grub-core/kern/i386/pc/init.c, grub-core/kern/mm.c and
> > grub-core/lib/relocator.c. At first sight it looks that
> > grub-core/kern/i386/pc/init.c:grub_machine_init() is the
> > most interesting thing.
> >
> > I hope that helps. If you have further questions drop me a line.
> >
> > Thank you for taking a stab at this.
>
> The whole lower 1M physical memory is reserved and not available for use
> (done by grub-core/kern/i386/pc/init.c), but effectively ignored by
> grub_relocator_alloc_chunk_addr for the non relocatable case during
> multiboot segment load.
>
> The adjust_limits call in grub_relocator_alloc_chunk_addr mainly sets
> the min_addr to 0 and max_addr to end of physical memory. So
> grub_relocator_alloc_chunk_addr will ever find a piece of (higher)
> memory, mainly ignoring the target address (which is reserved, e.g.
> below 1M).
>
> The showcase code snippet below catches the case now. I don't know
> whether it is correct nor do I like it ...
>
> What do you think ?
>
>
> Alex
>
>
> --- a/grub-core/lib/relocator.c
> +++ b/grub-core/lib/relocator.c
> @@ -1226,7 +1237,7 @@ adjust_limits (struct grub_relocator *rel,
>  grub_err_t
>  grub_relocator_alloc_chunk_addr (struct grub_relocator *rel,
>  				 grub_relocator_chunk_t *out,
> -				 grub_phys_addr_t target, grub_size_t size)
> +				 grub_phys_addr_t target, grub_size_t size, int relocatable)
>  {
>    struct grub_relocator_chunk *chunk;
>    grub_phys_addr_t min_addr = 0, max_addr;
> @@ -1250,6 +1263,10 @@ grub_relocator_alloc_chunk_addr (struct
> grub_relocator *rel,
>  		(unsigned long long) min_addr, (unsigned long long) max_addr,
>  		(unsigned long long) target);
>
> +  if (!relocatable &&
> +      !malloc_in_range (rel, target, target + size, 1, size, chunk, 0, 1))
> +    return grub_error (GRUB_ERR_BUG, "target memory not available");
> +
>    do
>      {
>        /* A trick to improve Linux allocation.  */

AIUI grub_relocator_alloc_chunk_addr() should allocate memory region
with exact properties, i.e, target and size. So, I think that relocatable
argument does not make sense here. However, I have a feeling that this
piece of code is a problem:

1255          /* A trick to improve Linux allocation.  */
1256    #if defined (__i386__) || defined (__x86_64__)
1257          if (target < 0x100000)
1258            if (malloc_in_range (rel, rel->highestnonpostaddr, ~(grub_addr_t)0, 1,
1259                                 size, chunk, 0, 1))
1260              {
1261                if (rel->postchunks > chunk->src)
1262                  rel->postchunks = chunk->src;
1263                break;
1264              }
1265    #endif

There is a chance that if you comment it out then Multiboot/Multiboot2
will work as expected. If it is true then we have to think how to
fix it and do not break Linux boot.

Daniel