On Tuesday, March 15, 2016, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
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:
> PHDR 1M-2M
> PHDR 2M-5M
> 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?
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
>

-- 
Regards
Vladimir 'phcoder' Serbinenko