[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images

On Tuesday, March 15, 2016, Vladimir 'phcoder' Serbinenko <phcoder@xxxxxxxxx> wrote:

+           if (mld->relocatable)
+             err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator, &ch,
+                                                     mld->min_addr, mld->max_addr - phdr(i)->p_memsz,
+                                                     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);
I believe this is faulty if you have more than one PHDR. You load every PHDR individually to essentially random address. Pieces have no reasonable way to find each other. Moreover entry point calculation is also faulty. Imagine sth like this:
Entry point 2.5M (in second PHDR)
then if first PHDR is loaded to 1M and second to 10M then base and link addr are both 1M, so entry point will be calculated as 2.5M, which points to no segment. I see 2 solutions:
1) Look where entry falls in original layout, then adjust it in accordance with where this phdr will be loaded. This requires least efforts. Finding different PHDRs is still impossible but it will be possible in the future with relocations.
2) Allocate a buffer of size highest - lowest and load everything into this buffer keeping relative offsets. If we do this, then we need to document if it's required for boorloader to behave this way or not. If it is, we can in future provide a tag to say that image is fine with rearrangement of PHDR, if it ever becomes relevant (I heavily doubt it).
I guess that xen is a single phdr image and so essentially any code will work with it. 
This problem appears in couple of other places, I'll skip commenting on them explicitly.
I take back the part "requires least effort" for solution 1. Solution 2 is probably simpler and less error-prone as developper doesn't control if binutils decode to put several phdrs. 
+  if (mld.relocatable)
+    {
+      if (mld.load_base_addr >= mld.link_base_addr)
+       grub_multiboot_payload_eip += mld.load_base_addr - mld.link_base_addr;
+      else
+       grub_multiboot_payload_eip -= mld.link_base_addr - mld.load_base_addr;
+    }
Both branches are mathematically equivalent. Any reason to have if at all? 

Vladimir 'phcoder' Serbinenko

Vladimir 'phcoder' Serbinenko

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.