Re: ELF bugfixes

[phcoder]

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

> > > > -  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

--

Regards
Vladimir 'phcoder' Serbinenko