Re: ELF bugfixes

Re: ELF bugfixes

[phcoder]

David Miller wrote:
> From: phcoder <phcoder@gmail.com>
> Date: Thu, 12 Mar 2009 14:21:45 +0100
> 
>> I knew it but normally when you parse files normally offsets aren't
>> guaranteed to be aligned. But now it seems that elf parser is
>> written in a way to guarantee at least some alignments. Then this
>> part of patch probably is to be dropped or changed to proper aligned
>> attribute
> 
> These ELF structures DO NOT need the aligned attribute either.
> 
> Nobody, in any source tree anyways, adds either the aligned
> or packed attributes to the core ELF data structures because
> they are absolutely not necessary.  And this includes source
> trees that support basically every CPU type out there.
> 
> Please stop making arbitrary changes, and instead describe on the
> mailing list the exact problem you are trying to solve.  Then,
> implement the solution which you can show is necessary and fully
> understand.
Actually what I was doing now was discussing. If we don't discuss we may 
everyone create our own fork. I previously had problems because some of 
the structures in headers didn't have proper alignment attribute. My 
problem was that grub2 didn't load solaris kernel. Further it revealed 
that the problem wasn't the attributes. I fixed the problem (see the 
rest of my patch) however I decided to submit also that part to prevent 
potential problems in the future. If you're expert with ELF format and 
say that this part will never be needed, it's ok with me but please stay 
respectful and discuss instead of accusing.

-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: ELF bugfixes

[David Miller]

From: phcoder <phcoder@gmail.com>
Date: Thu, 12 Mar 2009 14:43:51 +0100

> Actually what I was doing now was discussing. If we don't discuss we
> may everyone create our own fork. I previously had problems because
> some of the structures in headers didn't have proper alignment
> attribute. My problem was that grub2 didn't load solaris
> kernel. Further it revealed that the problem wasn't the
> attributes. I fixed the problem (see the rest of my patch) however I
> decided to submit also that part to prevent potential problems in
> the future. If you're expert with ELF format and say that this part
> will never be needed, it's ok with me but please stay respectful and
> discuss instead of accusing.

Your type attribute changes look arbitrary because you do not attach
appropriate detailed descriptions as to why you are making them.

They really aren't needed on any system.

If you require them to get things working, there is likely a problem
elsehwere.

ELF bugfixes

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 105 bytes --]

Hello I discovered some bugs in multiboot-elf. Here is bugfix
-- 

Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: mbelf.diff --]
[-- Type: text/x-patch, Size: 11742 bytes --]

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 2010)
+++ ChangeLog	(working copy)
@@ -1,3 +1,12 @@
+2009-03-01  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	Bugfixes in multiboot for bugs uncovered by solaris kernel
+
+	* loader/i386/multiboot_elfxx.c (grub_multiboot_load_elf): corrected 
+	limit detection
+	Use paddr for entry_point since kernel is started in physical mode
+	* include/grub/elf.h: added missing attributes
+
 2009-03-01  Bean  <bean123ch@gmail.com>
 
 	* include/grub/efi/api.h (GRUB_EFI_MPS_TABALE_GUID): New constant.
Index: include/grub/elf.h
===================================================================
--- include/grub/elf.h	(revision 2010)
+++ include/grub/elf.h	(working copy)
@@ -77,7 +77,7 @@
   Elf32_Half	e_shentsize;		/* Section header table entry size */
   Elf32_Half	e_shnum;		/* Section header table entry count */
   Elf32_Half	e_shstrndx;		/* Section header string table index */
-} Elf32_Ehdr;
+} __attribute__ ((packed)) Elf32_Ehdr;
 
 typedef struct
 {
@@ -95,7 +95,7 @@
   Elf64_Half	e_shentsize;		/* Section header table entry size */
   Elf64_Half	e_shnum;		/* Section header table entry count */
   Elf64_Half	e_shstrndx;		/* Section header string table index */
-} Elf64_Ehdr;
+} __attribute__ ((packed)) Elf64_Ehdr;
 
 /* Fields in the e_ident array.  The EI_* macros are indices into the
    array.  The macros under each EI_* macro are the values the byte
@@ -272,7 +272,7 @@
   Elf32_Word	sh_info;		/* Additional section information */
   Elf32_Word	sh_addralign;		/* Section alignment */
   Elf32_Word	sh_entsize;		/* Entry size if section holds table */
-} Elf32_Shdr;
+} __attribute__ ((packed)) Elf32_Shdr;
 
 typedef struct
 {
@@ -286,7 +286,7 @@
   Elf64_Word	sh_info;		/* Additional section information */
   Elf64_Xword	sh_addralign;		/* Section alignment */
   Elf64_Xword	sh_entsize;		/* Entry size if section holds table */
-} Elf64_Shdr;
+} __attribute__ ((packed)) Elf64_Shdr;
 
 /* Special section indices.  */
 
@@ -367,7 +367,7 @@
   unsigned char	st_info;		/* Symbol type and binding */
   unsigned char	st_other;		/* Symbol visibility */
   Elf32_Section	st_shndx;		/* Section index */
-} Elf32_Sym;
+} __attribute__ ((packed)) Elf32_Sym;
 
 typedef struct
 {
@@ -377,7 +377,7 @@
   Elf64_Section	st_shndx;		/* Section index */
   Elf64_Addr	st_value;		/* Symbol value */
   Elf64_Xword	st_size;		/* Symbol size */
-} Elf64_Sym;
+} __attribute__ ((packed)) Elf64_Sym;
 
 /* The syminfo section if available contains additional information about
    every dynamic symbol.  */
@@ -386,13 +386,13 @@
 {
   Elf32_Half si_boundto;		/* Direct bindings, symbol bound to */
   Elf32_Half si_flags;			/* Per symbol flags */
-} Elf32_Syminfo;
+} __attribute__ ((packed)) Elf32_Syminfo;
 
 typedef struct
 {
   Elf64_Half si_boundto;		/* Direct bindings, symbol bound to */
   Elf64_Half si_flags;			/* Per symbol flags */
-} Elf64_Syminfo;
+} __attribute__ ((packed)) Elf64_Syminfo;
 
 /* Possible values for si_boundto.  */
 #define SYMINFO_BT_SELF		0xffff	/* Symbol bound to self */
@@ -477,7 +477,7 @@
 {
   Elf32_Addr	r_offset;		/* Address */
   Elf32_Word	r_info;			/* Relocation type and symbol index */
-} Elf32_Rel;
+} __attribute__ ((packed)) Elf32_Rel;
 
 /* I have seen two different definitions of the Elf64_Rel and
    Elf64_Rela structures, so we'll leave them out until Novell (or
@@ -488,7 +488,7 @@
 {
   Elf64_Addr	r_offset;		/* Address */
   Elf64_Xword	r_info;			/* Relocation type and symbol index */
-} Elf64_Rel;
+} __attribute__ ((packed)) Elf64_Rel;
 
 /* Relocation table entry with addend (in section of type SHT_RELA).  */
 
@@ -497,14 +497,14 @@
   Elf32_Addr	r_offset;		/* Address */
   Elf32_Word	r_info;			/* Relocation type and symbol index */
   Elf32_Sword	r_addend;		/* Addend */
-} Elf32_Rela;
+} __attribute__ ((packed)) Elf32_Rela;
 
 typedef struct
 {
   Elf64_Addr	r_offset;		/* Address */
   Elf64_Xword	r_info;			/* Relocation type and symbol index */
   Elf64_Sxword	r_addend;		/* Addend */
-} Elf64_Rela;
+} __attribute__ ((packed)) Elf64_Rela;
 
 /* How to extract and insert information held in the r_info field.  */
 
@@ -528,7 +528,7 @@
   Elf32_Word	p_memsz;		/* Segment size in memory */
   Elf32_Word	p_flags;		/* Segment flags */
   Elf32_Word	p_align;		/* Segment alignment */
-} Elf32_Phdr;
+} __attribute__ ((packed)) Elf32_Phdr;
 
 typedef struct
 {
@@ -540,7 +540,7 @@
   Elf64_Xword	p_filesz;		/* Segment size in file */
   Elf64_Xword	p_memsz;		/* Segment size in memory */
   Elf64_Xword	p_align;		/* Segment alignment */
-} Elf64_Phdr;
+} __attribute__ ((packed)) Elf64_Phdr;
 
 /* Legal values for p_type (segment type).  */
 
@@ -604,7 +604,7 @@
       Elf32_Word d_val;			/* Integer value */
       Elf32_Addr d_ptr;			/* Address value */
     } d_un;
-} Elf32_Dyn;
+} __attribute__ ((packed)) Elf32_Dyn;
 
 typedef struct
 {
@@ -614,7 +614,7 @@
       Elf64_Xword d_val;		/* Integer value */
       Elf64_Addr d_ptr;			/* Address value */
     } d_un;
-} Elf64_Dyn;
+} __attribute__ ((packed)) Elf64_Dyn;
 
 /* Legal values for d_tag (dynamic entry type).  */
 
@@ -770,7 +770,7 @@
   Elf32_Word	vd_aux;			/* Offset in bytes to verdaux array */
   Elf32_Word	vd_next;		/* Offset in bytes to next verdef
 					   entry */
-} Elf32_Verdef;
+} __attribute__ ((packed)) Elf32_Verdef;
 
 typedef struct
 {
@@ -782,7 +782,7 @@
   Elf64_Word	vd_aux;			/* Offset in bytes to verdaux array */
   Elf64_Word	vd_next;		/* Offset in bytes to next verdef
 					   entry */
-} Elf64_Verdef;
+} __attribute__ ((packed)) Elf64_Verdef;
 
 
 /* Legal values for vd_version (version revision).  */
@@ -807,14 +807,14 @@
   Elf32_Word	vda_name;		/* Version or dependency names */
   Elf32_Word	vda_next;		/* Offset in bytes to next verdaux
 					   entry */
-} Elf32_Verdaux;
+} __attribute__ ((packed)) Elf32_Verdaux;
 
 typedef struct
 {
   Elf64_Word	vda_name;		/* Version or dependency names */
   Elf64_Word	vda_next;		/* Offset in bytes to next verdaux
 					   entry */
-} Elf64_Verdaux;
+} __attribute__ ((packed)) Elf64_Verdaux;
 
 
 /* Version dependency section.  */
@@ -828,7 +828,7 @@
   Elf32_Word	vn_aux;			/* Offset in bytes to vernaux array */
   Elf32_Word	vn_next;		/* Offset in bytes to next verneed
 					   entry */
-} Elf32_Verneed;
+} __attribute__ ((packed)) Elf32_Verneed;
 
 typedef struct
 {
@@ -839,7 +839,7 @@
   Elf64_Word	vn_aux;			/* Offset in bytes to vernaux array */
   Elf64_Word	vn_next;		/* Offset in bytes to next verneed
 					   entry */
-} Elf64_Verneed;
+} __attribute__ ((packed)) Elf64_Verneed;
 
 
 /* Legal values for vn_version (version revision).  */
@@ -857,7 +857,7 @@
   Elf32_Word	vna_name;		/* Dependency name string offset */
   Elf32_Word	vna_next;		/* Offset in bytes to next vernaux
 					   entry */
-} Elf32_Vernaux;
+} __attribute__ ((packed)) Elf32_Vernaux;
 
 typedef struct
 {
@@ -867,7 +867,7 @@
   Elf64_Word	vna_name;		/* Dependency name string offset */
   Elf64_Word	vna_next;		/* Offset in bytes to next vernaux
 					   entry */
-} Elf64_Vernaux;
+} __attribute__ ((packed)) Elf64_Vernaux;
 
 
 /* Legal values for vna_flags.  */
@@ -892,7 +892,7 @@
       void *a_ptr;		/* Pointer value */
       void (*a_fcn) (void);	/* Function pointer value */
     } a_un;
-} Elf32_auxv_t;
+} __attribute__ ((packed)) Elf32_auxv_t;
 
 typedef struct
 {
@@ -903,7 +903,7 @@
       void *a_ptr;		/* Pointer value */
       void (*a_fcn) (void);	/* Function pointer value */
     } a_un;
-} Elf64_auxv_t;
+} __attribute__ ((packed)) Elf64_auxv_t;
 
 /* Legal values for a_type (entry type).  */
 
@@ -951,14 +951,14 @@
   Elf32_Word n_namesz;			/* Length of the note's name.  */
   Elf32_Word n_descsz;			/* Length of the note's descriptor.  */
   Elf32_Word n_type;			/* Type of the note.  */
-} Elf32_Nhdr;
+} __attribute__ ((packed)) Elf32_Nhdr;
 
 typedef struct
 {
   Elf64_Word n_namesz;			/* Length of the note's name.  */
   Elf64_Word n_descsz;			/* Length of the note's descriptor.  */
   Elf64_Word n_type;			/* Type of the note.  */
-} Elf64_Nhdr;
+} __attribute__ ((packed)) Elf64_Nhdr;
 
 /* Known names of notes.  */
 
@@ -1000,7 +1000,7 @@
   Elf32_Word m_poffset;		/* Symbol offset.  */
   Elf32_Half m_repeat;		/* Repeat count.  */
   Elf32_Half m_stride;		/* Stride info.  */
-} Elf32_Move;
+} __attribute__ ((packed)) Elf32_Move;
 
 typedef struct
 {
@@ -1009,7 +1009,7 @@
   Elf64_Xword m_poffset;	/* Symbol offset.  */
   Elf64_Half m_repeat;		/* Repeat count.  */
   Elf64_Half m_stride;		/* Stride info.  */
-} Elf64_Move;
+} __attribute__ ((packed)) Elf64_Move;
 
 /* Macro to construct move records.  */
 #define ELF32_M_SYM(info)	((info) >> 8)
@@ -1369,7 +1369,7 @@
       Elf32_Word gt_g_value;		/* If this value were used for -G */
       Elf32_Word gt_bytes;		/* This many bytes would be used */
     } gt_entry;				/* Subsequent entries in section */
-} Elf32_gptab;
+} __attribute__ ((packed)) Elf32_gptab;
 
 /* Entry found in sections of type SHT_MIPS_REGINFO.  */
 
@@ -1378,7 +1378,7 @@
   Elf32_Word	ri_gprmask;		/* General registers used */
   Elf32_Word	ri_cprmask[4];		/* Coprocessor registers used */
   Elf32_Sword	ri_gp_value;		/* $gp register value */
-} Elf32_RegInfo;
+} __attribute__ ((packed)) Elf32_RegInfo;
 
 /* Entries found in sections of type SHT_MIPS_OPTIONS.  */
 
@@ -1390,7 +1390,7 @@
   Elf32_Section section;	/* Section header index of section affected,
 				   0 for global options.  */
   Elf32_Word info;		/* Kind-specific information.  */
-} Elf_Options;
+} __attribute__ ((packed)) Elf_Options;
 
 /* Values for `kind' field in Elf_Options.  */
 
@@ -1437,7 +1437,7 @@
 {
   Elf32_Word hwp_flags1;	/* Extra flags.  */
   Elf32_Word hwp_flags2;	/* Extra flags.  */
-} Elf_Options_Hw;
+} __attribute__ ((packed)) Elf_Options_Hw;
 
 /* Masks for `info' in ElfOptions for ODK_HWAND and ODK_HWOR entries.  */
 
@@ -1579,7 +1579,7 @@
   Elf32_Word l_checksum;	/* Checksum */
   Elf32_Word l_version;		/* Interface version */
   Elf32_Word l_flags;		/* Flags */
-} Elf32_Lib;
+} __attribute__ ((packed)) Elf32_Lib;
 
 typedef struct
 {
@@ -1588,7 +1588,7 @@
   Elf64_Word l_checksum;	/* Checksum */
   Elf64_Word l_version;		/* Interface version */
   Elf64_Word l_flags;		/* Flags */
-} Elf64_Lib;
+} __attribute__ ((packed)) Elf64_Lib;
 
 
 /* Legal values for l_flags.  */
Index: loader/i386/multiboot_elfxx.c
===================================================================
--- loader/i386/multiboot_elfxx.c	(revision 2010)
+++ loader/i386/multiboot_elfxx.c	(working copy)
@@ -49,7 +49,7 @@
 {
   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,14 @@
   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;
       }
+
   grub_multiboot_payload_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;
 
@@ -123,8 +126,9 @@
         }
     }
 
-  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;
 
+
 #undef phdr
 
   return grub_errno;

Re: ELF bugfixes

[Robert Millan]

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?

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

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

Re: ELF bugfixes

[phcoder]

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

Regards
Vladimir 'phcoder' Serbinenko

Re: ELF bugfixes

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

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


-- 

Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: elffixes.diff --]
[-- Type: text/x-diff, Size: 12340 bytes --]

Index: include/grub/elf.h
===================================================================
--- include/grub/elf.h	(revision 2036)
+++ include/grub/elf.h	(working copy)
@@ -77,7 +77,7 @@
   Elf32_Half	e_shentsize;		/* Section header table entry size */
   Elf32_Half	e_shnum;		/* Section header table entry count */
   Elf32_Half	e_shstrndx;		/* Section header string table index */
-} Elf32_Ehdr;
+} __attribute__ ((packed)) Elf32_Ehdr;
 
 typedef struct
 {
@@ -95,7 +95,7 @@
   Elf64_Half	e_shentsize;		/* Section header table entry size */
   Elf64_Half	e_shnum;		/* Section header table entry count */
   Elf64_Half	e_shstrndx;		/* Section header string table index */
-} Elf64_Ehdr;
+} __attribute__ ((packed)) Elf64_Ehdr;
 
 /* Fields in the e_ident array.  The EI_* macros are indices into the
    array.  The macros under each EI_* macro are the values the byte
@@ -272,7 +272,7 @@
   Elf32_Word	sh_info;		/* Additional section information */
   Elf32_Word	sh_addralign;		/* Section alignment */
   Elf32_Word	sh_entsize;		/* Entry size if section holds table */
-} Elf32_Shdr;
+} __attribute__ ((packed)) Elf32_Shdr;
 
 typedef struct
 {
@@ -286,7 +286,7 @@
   Elf64_Word	sh_info;		/* Additional section information */
   Elf64_Xword	sh_addralign;		/* Section alignment */
   Elf64_Xword	sh_entsize;		/* Entry size if section holds table */
-} Elf64_Shdr;
+} __attribute__ ((packed)) Elf64_Shdr;
 
 /* Special section indices.  */
 
@@ -367,7 +367,7 @@
   unsigned char	st_info;		/* Symbol type and binding */
   unsigned char	st_other;		/* Symbol visibility */
   Elf32_Section	st_shndx;		/* Section index */
-} Elf32_Sym;
+} __attribute__ ((packed)) Elf32_Sym;
 
 typedef struct
 {
@@ -377,7 +377,7 @@
   Elf64_Section	st_shndx;		/* Section index */
   Elf64_Addr	st_value;		/* Symbol value */
   Elf64_Xword	st_size;		/* Symbol size */
-} Elf64_Sym;
+} __attribute__ ((packed)) Elf64_Sym;
 
 /* The syminfo section if available contains additional information about
    every dynamic symbol.  */
@@ -386,13 +386,13 @@
 {
   Elf32_Half si_boundto;		/* Direct bindings, symbol bound to */
   Elf32_Half si_flags;			/* Per symbol flags */
-} Elf32_Syminfo;
+} __attribute__ ((packed)) Elf32_Syminfo;
 
 typedef struct
 {
   Elf64_Half si_boundto;		/* Direct bindings, symbol bound to */
   Elf64_Half si_flags;			/* Per symbol flags */
-} Elf64_Syminfo;
+} __attribute__ ((packed)) Elf64_Syminfo;
 
 /* Possible values for si_boundto.  */
 #define SYMINFO_BT_SELF		0xffff	/* Symbol bound to self */
@@ -477,7 +477,7 @@
 {
   Elf32_Addr	r_offset;		/* Address */
   Elf32_Word	r_info;			/* Relocation type and symbol index */
-} Elf32_Rel;
+} __attribute__ ((packed)) Elf32_Rel;
 
 /* I have seen two different definitions of the Elf64_Rel and
    Elf64_Rela structures, so we'll leave them out until Novell (or
@@ -488,7 +488,7 @@
 {
   Elf64_Addr	r_offset;		/* Address */
   Elf64_Xword	r_info;			/* Relocation type and symbol index */
-} Elf64_Rel;
+} __attribute__ ((packed)) Elf64_Rel;
 
 /* Relocation table entry with addend (in section of type SHT_RELA).  */
 
@@ -497,14 +497,14 @@
   Elf32_Addr	r_offset;		/* Address */
   Elf32_Word	r_info;			/* Relocation type and symbol index */
   Elf32_Sword	r_addend;		/* Addend */
-} Elf32_Rela;
+} __attribute__ ((packed)) Elf32_Rela;
 
 typedef struct
 {
   Elf64_Addr	r_offset;		/* Address */
   Elf64_Xword	r_info;			/* Relocation type and symbol index */
   Elf64_Sxword	r_addend;		/* Addend */
-} Elf64_Rela;
+} __attribute__ ((packed)) Elf64_Rela;
 
 /* How to extract and insert information held in the r_info field.  */
 
@@ -528,7 +528,7 @@
   Elf32_Word	p_memsz;		/* Segment size in memory */
   Elf32_Word	p_flags;		/* Segment flags */
   Elf32_Word	p_align;		/* Segment alignment */
-} Elf32_Phdr;
+} __attribute__ ((packed)) Elf32_Phdr;
 
 typedef struct
 {
@@ -540,7 +540,7 @@
   Elf64_Xword	p_filesz;		/* Segment size in file */
   Elf64_Xword	p_memsz;		/* Segment size in memory */
   Elf64_Xword	p_align;		/* Segment alignment */
-} Elf64_Phdr;
+} __attribute__ ((packed)) Elf64_Phdr;
 
 /* Legal values for p_type (segment type).  */
 
@@ -604,7 +604,7 @@
       Elf32_Word d_val;			/* Integer value */
       Elf32_Addr d_ptr;			/* Address value */
     } d_un;
-} Elf32_Dyn;
+} __attribute__ ((packed)) Elf32_Dyn;
 
 typedef struct
 {
@@ -614,7 +614,7 @@
       Elf64_Xword d_val;		/* Integer value */
       Elf64_Addr d_ptr;			/* Address value */
     } d_un;
-} Elf64_Dyn;
+} __attribute__ ((packed)) Elf64_Dyn;
 
 /* Legal values for d_tag (dynamic entry type).  */
 
@@ -770,7 +770,7 @@
   Elf32_Word	vd_aux;			/* Offset in bytes to verdaux array */
   Elf32_Word	vd_next;		/* Offset in bytes to next verdef
 					   entry */
-} Elf32_Verdef;
+} __attribute__ ((packed)) Elf32_Verdef;
 
 typedef struct
 {
@@ -782,7 +782,7 @@
   Elf64_Word	vd_aux;			/* Offset in bytes to verdaux array */
   Elf64_Word	vd_next;		/* Offset in bytes to next verdef
 					   entry */
-} Elf64_Verdef;
+} __attribute__ ((packed)) Elf64_Verdef;
 
 
 /* Legal values for vd_version (version revision).  */
@@ -807,14 +807,14 @@
   Elf32_Word	vda_name;		/* Version or dependency names */
   Elf32_Word	vda_next;		/* Offset in bytes to next verdaux
 					   entry */
-} Elf32_Verdaux;
+} __attribute__ ((packed)) Elf32_Verdaux;
 
 typedef struct
 {
   Elf64_Word	vda_name;		/* Version or dependency names */
   Elf64_Word	vda_next;		/* Offset in bytes to next verdaux
 					   entry */
-} Elf64_Verdaux;
+} __attribute__ ((packed)) Elf64_Verdaux;
 
 
 /* Version dependency section.  */
@@ -828,7 +828,7 @@
   Elf32_Word	vn_aux;			/* Offset in bytes to vernaux array */
   Elf32_Word	vn_next;		/* Offset in bytes to next verneed
 					   entry */
-} Elf32_Verneed;
+} __attribute__ ((packed)) Elf32_Verneed;
 
 typedef struct
 {
@@ -839,7 +839,7 @@
   Elf64_Word	vn_aux;			/* Offset in bytes to vernaux array */
   Elf64_Word	vn_next;		/* Offset in bytes to next verneed
 					   entry */
-} Elf64_Verneed;
+} __attribute__ ((packed)) Elf64_Verneed;
 
 
 /* Legal values for vn_version (version revision).  */
@@ -857,7 +857,7 @@
   Elf32_Word	vna_name;		/* Dependency name string offset */
   Elf32_Word	vna_next;		/* Offset in bytes to next vernaux
 					   entry */
-} Elf32_Vernaux;
+} __attribute__ ((packed)) Elf32_Vernaux;
 
 typedef struct
 {
@@ -867,7 +867,7 @@
   Elf64_Word	vna_name;		/* Dependency name string offset */
   Elf64_Word	vna_next;		/* Offset in bytes to next vernaux
 					   entry */
-} Elf64_Vernaux;
+} __attribute__ ((packed)) Elf64_Vernaux;
 
 
 /* Legal values for vna_flags.  */
@@ -892,7 +892,7 @@
       void *a_ptr;		/* Pointer value */
       void (*a_fcn) (void);	/* Function pointer value */
     } a_un;
-} Elf32_auxv_t;
+} __attribute__ ((packed)) Elf32_auxv_t;
 
 typedef struct
 {
@@ -903,7 +903,7 @@
       void *a_ptr;		/* Pointer value */
       void (*a_fcn) (void);	/* Function pointer value */
     } a_un;
-} Elf64_auxv_t;
+} __attribute__ ((packed)) Elf64_auxv_t;
 
 /* Legal values for a_type (entry type).  */
 
@@ -951,14 +951,14 @@
   Elf32_Word n_namesz;			/* Length of the note's name.  */
   Elf32_Word n_descsz;			/* Length of the note's descriptor.  */
   Elf32_Word n_type;			/* Type of the note.  */
-} Elf32_Nhdr;
+} __attribute__ ((packed)) Elf32_Nhdr;
 
 typedef struct
 {
   Elf64_Word n_namesz;			/* Length of the note's name.  */
   Elf64_Word n_descsz;			/* Length of the note's descriptor.  */
   Elf64_Word n_type;			/* Type of the note.  */
-} Elf64_Nhdr;
+} __attribute__ ((packed)) Elf64_Nhdr;
 
 /* Known names of notes.  */
 
@@ -1000,7 +1000,7 @@
   Elf32_Word m_poffset;		/* Symbol offset.  */
   Elf32_Half m_repeat;		/* Repeat count.  */
   Elf32_Half m_stride;		/* Stride info.  */
-} Elf32_Move;
+} __attribute__ ((packed)) Elf32_Move;
 
 typedef struct
 {
@@ -1009,7 +1009,7 @@
   Elf64_Xword m_poffset;	/* Symbol offset.  */
   Elf64_Half m_repeat;		/* Repeat count.  */
   Elf64_Half m_stride;		/* Stride info.  */
-} Elf64_Move;
+} __attribute__ ((packed)) Elf64_Move;
 
 /* Macro to construct move records.  */
 #define ELF32_M_SYM(info)	((info) >> 8)
@@ -1369,7 +1369,7 @@
       Elf32_Word gt_g_value;		/* If this value were used for -G */
       Elf32_Word gt_bytes;		/* This many bytes would be used */
     } gt_entry;				/* Subsequent entries in section */
-} Elf32_gptab;
+} __attribute__ ((packed)) Elf32_gptab;
 
 /* Entry found in sections of type SHT_MIPS_REGINFO.  */
 
@@ -1378,7 +1378,7 @@
   Elf32_Word	ri_gprmask;		/* General registers used */
   Elf32_Word	ri_cprmask[4];		/* Coprocessor registers used */
   Elf32_Sword	ri_gp_value;		/* $gp register value */
-} Elf32_RegInfo;
+} __attribute__ ((packed)) Elf32_RegInfo;
 
 /* Entries found in sections of type SHT_MIPS_OPTIONS.  */
 
@@ -1390,7 +1390,7 @@
   Elf32_Section section;	/* Section header index of section affected,
 				   0 for global options.  */
   Elf32_Word info;		/* Kind-specific information.  */
-} Elf_Options;
+} __attribute__ ((packed)) Elf_Options;
 
 /* Values for `kind' field in Elf_Options.  */
 
@@ -1437,7 +1437,7 @@
 {
   Elf32_Word hwp_flags1;	/* Extra flags.  */
   Elf32_Word hwp_flags2;	/* Extra flags.  */
-} Elf_Options_Hw;
+} __attribute__ ((packed)) Elf_Options_Hw;
 
 /* Masks for `info' in ElfOptions for ODK_HWAND and ODK_HWOR entries.  */
 
@@ -1579,7 +1579,7 @@
   Elf32_Word l_checksum;	/* Checksum */
   Elf32_Word l_version;		/* Interface version */
   Elf32_Word l_flags;		/* Flags */
-} Elf32_Lib;
+} __attribute__ ((packed)) Elf32_Lib;
 
 typedef struct
 {
@@ -1588,7 +1588,7 @@
   Elf64_Word l_checksum;	/* Checksum */
   Elf64_Word l_version;		/* Interface version */
   Elf64_Word l_flags;		/* Flags */
-} Elf64_Lib;
+} __attribute__ ((packed)) Elf64_Lib;
 
 
 /* Legal values for l_flags.  */
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 2036)
+++ ChangeLog	(working copy)
@@ -1,3 +1,12 @@
+2009-03-01  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	Bugfixes in multiboot for bugs uncovered by solaris kernel
+
+	* loader/i386/multiboot_elfxx.c (grub_multiboot_load_elf): corrected 
+	limit detection
+	Use vaddr of correct segment for entry_point 
+	* include/grub/elf.h: added missing __attribute__ ((packed))
+
 2009-03-12  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	Parttool
Index: loader/i386/multiboot_elfxx.c
===================================================================
--- loader/i386/multiboot_elfxx.c	(revision 2036)
+++ loader/i386/multiboot_elfxx.c	(working copy)
@@ -49,7 +49,7 @@
 {
   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,9 +83,11 @@
   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;
       }
   code_size = (phdr(highest_segment)->p_paddr + phdr(highest_segment)->p_memsz) - phdr(lowest_segment)->p_paddr;
@@ -105,8 +107,8 @@
         {
 	  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 +126,11 @@
         }
     }
 
-  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);
 
 #undef phdr
 

Re: ELF bugfixes

[David Miller]

From: phcoder <phcoder@gmail.com>
Date: Thu, 12 Mar 2009 09:23:34 +0100

> Index: include/grub/elf.h
> ===================================================================
> --- include/grub/elf.h	(revision 2036)
> +++ include/grub/elf.h	(working copy)
> @@ -77,7 +77,7 @@
>    Elf32_Half	e_shentsize;		/* Section header table entry size */
>    Elf32_Half	e_shnum;		/* Section header table entry count */
>    Elf32_Half	e_shstrndx;		/* Section header string table index */
> -} Elf32_Ehdr;
> +} __attribute__ ((packed)) Elf32_Ehdr;
>  

There is no reason why you should need the packed attribute here.

I can't think of any cpu where this could even remotely be necessary.
And if it's not necessary, all it does it emit terribly suboptimal
code on RISC cpus.

Re: ELF bugfixes

[Robert Millan]

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?

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

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

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

Re: ELF bugfixes

[David Miller]

From: phcoder <phcoder@gmail.com>
Date: Fri, 13 Mar 2009 21:41:42 +0100

> 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 would suggest simply looping over the phdrs and remembering
which one the e_entry falls into.

Won't that make things work in the case you're describing?

Re: ELF bugfixes

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

David Miller wrote:
> From: phcoder <phcoder@gmail.com>
> Date: Fri, 13 Mar 2009 21:41:42 +0100
> 
>> 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 would suggest simply looping over the phdrs and remembering
> which one the e_entry falls into.
> 
> Won't that make things work in the case you're describing?
> 
I thought I have attached new patch. Sorry forgot to do so

-- 

Regards
Vladimir 'phcoder' Serbinenko


[-- Attachment #2: elffixes.diff --]
[-- Type: text/x-diff, Size: 2694 bytes --]

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 2036)
+++ ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2009-03-01  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	Bugfixes in multiboot for bugs uncovered by solaris kernel
+
+	* loader/i386/multiboot_elfxx.c (grub_multiboot_load_elf): corrected 
+	limit detection
+	Use vaddr of correct segment for entry_point 
+
 2009-03-12  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	Parttool
Index: loader/i386/multiboot_elfxx.c
===================================================================
--- loader/i386/multiboot_elfxx.c	(revision 2036)
+++ loader/i386/multiboot_elfxx.c	(working copy)
@@ -49,7 +49,7 @@
 {
   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,17 @@
   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;
       }
+
+  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 +111,8 @@
         {
 	  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 +130,11 @@
         }
     }
 
-  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);
 
 #undef phdr
 

Re: ELF bugfixes

[Robert Millan]

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

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

Re: ELF bugfixes

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

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

-- 

Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: elf.diff --]
[-- Type: text/x-diff, Size: 2661 bytes --]

diff --git a/loader/i386/multiboot_elfxx.c b/loader/i386/multiboot_elfxx.c
index 801800c..706d44d 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,11 +131,18 @@ 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);
+	return grub_errno;
+      }
 
 #undef phdr
 
-  return grub_errno;
+  return grub_error (GRUB_ERR_BAD_OS, "entry point isn't in a segment");
 }
 
 #undef XX

Re: ELF bugfixes

[Robert Millan]

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.

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

Re: ELF bugfixes

[phcoder]

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

-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: ELF bugfixes

[Robert Millan]

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.

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

Re: ELF bugfixes

[phcoder]

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

-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: ELF bugfixes

[Robert Millan]

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.

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

Re: ELF bugfixes

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

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

[-- Attachment #2: elf.diff --]
[-- Type: text/x-diff, Size: 2641 bytes --]

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
 

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
> Grub-devel@gnu.org
> 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."

Re: ELF bugfixes

[Robert Millan]

On Sat, Mar 21, 2009 at 11:03:12PM +0100, Robert Millan wrote:
> >>
> > 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.

My bad.  This' been clarified on IRC.  Sometimes I read code too quickly...

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

Re: ELF bugfixes

[Robert Millan]

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?

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

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

Re: ELF bugfixes

[phcoder]

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

Re: ELF bugfixes

[Robert Millan]

On Sat, Mar 14, 2009 at 12:01:13AM +0100, phcoder wrote:
> 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

Oh, of course...  This part of the patch is fine.  Perhaps a comment would be
a good idea, so we don't forget.

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

Re: ELF bugfixes

[phcoder]

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?
> 
>>>>>> -  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.
> 
Tested. Works fine

-- 

Regards
Vladimir 'phcoder' Serbinenko