Re: ELF bugfixes

[Robert Millan]

Committed.

On Sat, Mar 21, 2009 at 11:49:18PM +0100, phcoder wrote:
> Robert Millan wrote:
>> On Sat, Mar 21, 2009 at 07:05:23PM +0100, phcoder wrote:
>>> Robert Millan wrote:
>>>> On Sat, Mar 21, 2009 at 06:58:58PM +0100, phcoder wrote:
>>>>> Robert Millan wrote:
>>>>>> On Wed, Mar 18, 2009 at 02:26:40PM +0100, phcoder wrote:
>>>>>>> Robert Millan wrote:
>>>>>>>> On Fri, Mar 13, 2009 at 09:52:39PM +0100, phcoder wrote:
>>>>>>>>> -  grub_multiboot_payload_entry_offset = ehdr->e_entry - 
>>>>>>>>> phdr(lowest_segment)->p_vaddr;
>>>>>>>>> +  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)
>>>>>>>>> +      grub_multiboot_payload_entry_offset = (ehdr->e_entry - 
>>>>>>>>> phdr(i)->p_vaddr)
>>>>>>>>> +     + (phdr(i)->p_paddr  - phdr(lowest_segment)->p_paddr);
>>>>>>>> You need to handle the case in which 
>>>>>>>> grub_multiboot_payload_entry_offset is left
>>>>>>>> uninitialized (it needs to be initialized each time the multiboot 
>>>>>>>> command is
>>>>>>>> run, not just when the module is loaded).
>>>>>>>>
>>>>>>> module? actually it's when loading image. Perhaps you mean 
>>>>>>> that   additional error check is necessary
>>>>>> I meant GRUB's multiboot.mod, not the payload's module.  Sorry I wasn't 
>>>>>> clear.
>>>>>>
>>>>> With this error check if grub_multiboot_payload_entry_offset it 
>>>>> can   happen only if no image is loaded. And actually    
>>>>> grub_multiboot_payload_entry_offset is set to 0 at multiboot.mod 
>>>>> load
>>>>> So I don't really understand the problem
>>>> You can't rely on grub_multiboot_payload_entry_offset being set to 0, 
>>>> because
>>>> any subsequent call of "multiboot /something" has the potential to override
>>>> this.  You must not assume the multiboot command is only going to be run 
>>>> once.
>>>>
>>> No but it always corresponds to the current image. It's set either in 
>>>  multiboot.c or in grub_multiboot_load_elf
>>
>> It is now, but your code makes this conditional.
>>
>
>
> -- 
>
> Regards
> Vladimir 'phcoder' Serbinenko

> diff --git a/loader/i386/multiboot_elfxx.c b/loader/i386/multiboot_elfxx.c
> index 801800c..ce9e4fe 100644
> --- a/loader/i386/multiboot_elfxx.c
> +++ b/loader/i386/multiboot_elfxx.c
> @@ -49,7 +49,7 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, void 
> *buffer)
>  {
>    Elf_Ehdr *ehdr = (Elf_Ehdr *) buffer;
>    char *phdr_base;
> -  int lowest_segment = 0, highest_segment = 0;
> +  int lowest_segment = -1, highest_segment = -1;
>    int i;
>  
>    if (ehdr->e_ident[EI_CLASS] != ELFCLASSXX)
> @@ -83,11 +83,18 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, 
> void *buffer)
>    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)
> +     /* Beware that segment 0 isn't necessarily loadable */
> +     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;
>        }
> +
> +  if (lowest_segment == -1)
> +    return grub_error (GRUB_ERR_BAD_OS, "ELF contains no loadable segments");
> +
>    code_size = (phdr(highest_segment)->p_paddr + 
> phdr(highest_segment)->p_memsz) - phdr(lowest_segment)->p_paddr;
>    grub_multiboot_payload_dest = phdr(lowest_segment)->p_paddr;
>  
> @@ -105,8 +112,8 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, 
> void *buffer)
>          {
>         char *load_this_module_at = (char *) (grub_multiboot_payload_orig + 
> (long) (phdr(i)->p_paddr - phdr(lowest_segment)->p_paddr));
>  
> -       grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, 
> memsz=0x%lx\n",
> -                     i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz);
> +       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);
>  
>         if (grub_file_seek (file, (grub_off_t) phdr(i)->p_offset)
>             == (grub_off_t) -1)
> @@ -124,7 +131,17 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, 
> void *buffer)
>          }
>      }
>  
> -  grub_multiboot_payload_entry_offset = ehdr->e_entry - 
> phdr(lowest_segment)->p_vaddr;
> +  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)
> +      {
> +     grub_multiboot_payload_entry_offset = (ehdr->e_entry - phdr(i)->p_vaddr)
> +       + (phdr(i)->p_paddr  - phdr(lowest_segment)->p_paddr);
> +     break;
> +      }
> +
> +  if (i == ehdr->e_phnum)
> +    return grub_error (GRUB_ERR_BAD_OS, "entry point isn't in a segment");
>  
>  #undef phdr
>  

> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."