Re: ELF bugfixes

[phcoder <phcoder@gmail.com>]

Robert Millan wrote:
> On Fri, Mar 13, 2009 at 09:41:42PM +0100, phcoder wrote:
>> Robert Millan wrote:
>>> On Wed, Mar 11, 2009 at 10:21:41PM +0100, phcoder wrote:
>>>> Robert Millan wrote:
>>>>> On Mon, Mar 02, 2009 at 01:35:06AM +0100, phcoder wrote:
>>>>>> +	* include/grub/elf.h: added missing attributes
>>>>> This should be a bit more descriptive.
>>>>>
>>>>>>    for (i = 0; i < ehdr->e_phnum; i++)
>>>>>>      if (phdr(i)->p_type == PT_LOAD && phdr(i)->p_filesz != 0)
>>>>>>        {
>>>>>> -	if (phdr(i)->p_paddr < phdr(lowest_segment)->p_paddr)
>>>>>> +	if (lowest_segment == -1 +	    || phdr(i)->p_paddr <  
>>>>>> phdr(lowest_segment)->p_paddr)
>>>>>>  	  lowest_segment = i;
>>>>>> -	if (phdr(i)->p_paddr > phdr(highest_segment)->p_paddr)
>>>>>> +	if (highest_segment == -1
>>>>>> +	    || phdr(i)->p_paddr > phdr(highest_segment)->p_paddr)
>>>>>>  	  highest_segment = i;
>>>>>>        }
>>>>> Why?
>>>> Because if first segment doesn't have the PT_LOAD attribute set then 
>>>> it  should be considered in this comparison
>>> But you didn't remove the PT_LOAD check.  And in the routine below that
>>> does the actual segment load, we still check for PT_LOAD.  Those should be
>>> consistent, right?
>>>
>> No I expressed myself badly. Original code assumed that first segment  
>> has PT_LOAD always set (lowest_segment is 0 initally). I removed this  
>> assumption
> 
> Why do we care about non-PT_LOAD segments?

We don't but without this fix non-PT_LOAD segment 1 wasn't correctly ignored
> 
>>>>>> -  grub_multiboot_payload_entry_offset = ehdr->e_entry - phdr(lowest_segment)->p_vaddr;
>>>>>> +  grub_multiboot_payload_entry_offset = ehdr->e_entry - phdr(lowest_segment)->p_paddr;
>>>>> Are you sure about this?  IIRC e_entry is in the virtual address space.  I
>>>>> think we had some trouble with this (with NetBSD?), which lead to the current
>>>>> use of p_vaddr in this line.
>>>>>
>>>> Actually now thinking I see that the problem is more deep. The 
>>>> section  which is loaded at the lowest address isn't necessarily the 
>>>> section  which contains entry point. I'll fix this part cleanly and 
>>>> will resubmit  the patch
>>> No, but AFAICT the entry point is defined relative to that address, regardless
>>> of which segment contains it.
>>>
>> Actually our segment table is also our table for transforming between  
>> virtual and physical address. I don't see why entry point would be  
>> defined against virtual address of lowest physical segement
> 
> I think entry point is supposed to be defined in virtual address space.  As
> to why do we check for physical addresses earlier, I'm not entirely sure.  I
> think the idea was that we store the entry point as an offset, so that it
> can be applied to physical addresses, despite the fact that we obtained it
> by comparing e_entry with a virtual address.
> 
> ISTR this being an issue for NetBSD.  We should be certain what we do before
> changing it.  In particular, the following commit seem relevant:
> 
> 2008-02-05  Bean  <bean123ch@gmail.com>
> 
>         * loader/i386/pc/multiboot.c (grub_multiboot_load_elf32): Get physical
>         address of entry.
> 
> I'd also recommend testing your changes with NetBSD's kernel.
> 

Ok will do it
-- 

Regards
Vladimir 'phcoder' Serbinenko