Re: ELF bugfixes

[phcoder]

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

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  <address@hidden>
+
+       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  <address@hidden>
 
        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