* ELF bugfixes
@ 2009-03-02 0:35 phcoder
2009-03-11 21:15 ` Robert Millan
0 siblings, 1 reply; 25+ messages in thread
From: phcoder @ 2009-03-02 0:35 UTC (permalink / raw)
To: The development of GRUB 2
[-- 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;
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-02 0:35 ELF bugfixes phcoder
@ 2009-03-11 21:15 ` Robert Millan
2009-03-11 21:21 ` phcoder
0 siblings, 1 reply; 25+ messages in thread
From: Robert Millan @ 2009-03-11 21:15 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-11 21:15 ` Robert Millan
@ 2009-03-11 21:21 ` phcoder
2009-03-12 8:23 ` phcoder
2009-03-13 19:14 ` Robert Millan
0 siblings, 2 replies; 25+ messages in thread
From: phcoder @ 2009-03-11 21:21 UTC (permalink / raw)
To: The development of GRUB 2
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-11 21:21 ` phcoder
@ 2009-03-12 8:23 ` phcoder
2009-03-12 9:07 ` David Miller
2009-03-13 19:14 ` Robert Millan
1 sibling, 1 reply; 25+ messages in thread
From: phcoder @ 2009-03-12 8:23 UTC (permalink / raw)
To: The development of GRUB 2
[-- 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-12 8:23 ` phcoder
@ 2009-03-12 9:07 ` David Miller
0 siblings, 0 replies; 25+ messages in thread
From: David Miller @ 2009-03-12 9:07 UTC (permalink / raw)
To: grub-devel, phcoder
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-11 21:21 ` phcoder
2009-03-12 8:23 ` phcoder
@ 2009-03-13 19:14 ` Robert Millan
2009-03-13 20:41 ` phcoder
1 sibling, 1 reply; 25+ messages in thread
From: Robert Millan @ 2009-03-13 19:14 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-13 19:14 ` Robert Millan
@ 2009-03-13 20:41 ` phcoder
2009-03-13 20:45 ` David Miller
2009-03-13 22:46 ` Robert Millan
0 siblings, 2 replies; 25+ messages in thread
From: phcoder @ 2009-03-13 20:41 UTC (permalink / raw)
To: The development of GRUB 2
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-13 20:41 ` phcoder
@ 2009-03-13 20:45 ` David Miller
2009-03-13 20:52 ` phcoder
2009-03-13 22:46 ` Robert Millan
1 sibling, 1 reply; 25+ messages in thread
From: David Miller @ 2009-03-13 20:45 UTC (permalink / raw)
To: grub-devel, phcoder
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?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-13 20:45 ` David Miller
@ 2009-03-13 20:52 ` phcoder
2009-03-18 10:12 ` Robert Millan
0 siblings, 1 reply; 25+ messages in thread
From: phcoder @ 2009-03-13 20:52 UTC (permalink / raw)
To: The development of GRUB 2
[-- 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-13 20:41 ` phcoder
2009-03-13 20:45 ` David Miller
@ 2009-03-13 22:46 ` Robert Millan
2009-03-13 23:01 ` phcoder
2009-03-15 21:30 ` phcoder
1 sibling, 2 replies; 25+ messages in thread
From: Robert Millan @ 2009-03-13 22:46 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-13 22:46 ` Robert Millan
@ 2009-03-13 23:01 ` phcoder
2009-03-14 14:53 ` Robert Millan
2009-03-15 21:30 ` phcoder
1 sibling, 1 reply; 25+ messages in thread
From: phcoder @ 2009-03-13 23:01 UTC (permalink / raw)
To: The development of GRUB 2
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-13 23:01 ` phcoder
@ 2009-03-14 14:53 ` Robert Millan
0 siblings, 0 replies; 25+ messages in thread
From: Robert Millan @ 2009-03-14 14:53 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-13 22:46 ` Robert Millan
2009-03-13 23:01 ` phcoder
@ 2009-03-15 21:30 ` phcoder
1 sibling, 0 replies; 25+ messages in thread
From: phcoder @ 2009-03-15 21:30 UTC (permalink / raw)
To: The development of GRUB 2
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-13 20:52 ` phcoder
@ 2009-03-18 10:12 ` Robert Millan
2009-03-18 13:26 ` phcoder
0 siblings, 1 reply; 25+ messages in thread
From: Robert Millan @ 2009-03-18 10:12 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-18 10:12 ` Robert Millan
@ 2009-03-18 13:26 ` phcoder
2009-03-21 17:46 ` Robert Millan
0 siblings, 1 reply; 25+ messages in thread
From: phcoder @ 2009-03-18 13:26 UTC (permalink / raw)
To: The development of GRUB 2
[-- 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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-18 13:26 ` phcoder
@ 2009-03-21 17:46 ` Robert Millan
2009-03-21 17:58 ` phcoder
0 siblings, 1 reply; 25+ messages in thread
From: Robert Millan @ 2009-03-21 17:46 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-21 17:46 ` Robert Millan
@ 2009-03-21 17:58 ` phcoder
2009-03-21 18:03 ` Robert Millan
0 siblings, 1 reply; 25+ messages in thread
From: phcoder @ 2009-03-21 17:58 UTC (permalink / raw)
To: The development of GRUB 2
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-21 17:58 ` phcoder
@ 2009-03-21 18:03 ` Robert Millan
2009-03-21 18:05 ` phcoder
0 siblings, 1 reply; 25+ messages in thread
From: Robert Millan @ 2009-03-21 18:03 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-21 18:03 ` Robert Millan
@ 2009-03-21 18:05 ` phcoder
2009-03-21 22:03 ` Robert Millan
0 siblings, 1 reply; 25+ messages in thread
From: phcoder @ 2009-03-21 18:05 UTC (permalink / raw)
To: The development of GRUB 2
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-21 18:05 ` phcoder
@ 2009-03-21 22:03 ` Robert Millan
2009-03-21 22:49 ` phcoder
2009-03-21 22:55 ` Robert Millan
0 siblings, 2 replies; 25+ messages in thread
From: Robert Millan @ 2009-03-21 22:03 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-21 22:03 ` Robert Millan
@ 2009-03-21 22:49 ` phcoder
2009-03-21 23:02 ` Robert Millan
2009-03-21 22:55 ` Robert Millan
1 sibling, 1 reply; 25+ messages in thread
From: phcoder @ 2009-03-21 22:49 UTC (permalink / raw)
To: The development of GRUB 2
[-- 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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-21 22:03 ` Robert Millan
2009-03-21 22:49 ` phcoder
@ 2009-03-21 22:55 ` Robert Millan
1 sibling, 0 replies; 25+ messages in thread
From: Robert Millan @ 2009-03-21 22:55 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-21 22:49 ` phcoder
@ 2009-03-21 23:02 ` Robert Millan
0 siblings, 0 replies; 25+ messages in thread
From: Robert Millan @ 2009-03-21 23:02 UTC (permalink / raw)
To: The development of GRUB 2
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."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
2009-03-12 13:43 ` phcoder
@ 2009-03-12 14:05 ` David Miller
0 siblings, 0 replies; 25+ messages in thread
From: David Miller @ 2009-03-12 14:05 UTC (permalink / raw)
To: phcoder; +Cc: grub-devel
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: ELF bugfixes
[not found] ` <20090312.062628.260166400.davem@davemloft.net>
@ 2009-03-12 13:43 ` phcoder
2009-03-12 14:05 ` David Miller
0 siblings, 1 reply; 25+ messages in thread
From: phcoder @ 2009-03-12 13:43 UTC (permalink / raw)
To: David Miller, The development of GRUB 2
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
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2009-03-21 23:02 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-02 0:35 ELF bugfixes phcoder
2009-03-11 21:15 ` Robert Millan
2009-03-11 21:21 ` phcoder
2009-03-12 8:23 ` phcoder
2009-03-12 9:07 ` David Miller
2009-03-13 19:14 ` Robert Millan
2009-03-13 20:41 ` phcoder
2009-03-13 20:45 ` David Miller
2009-03-13 20:52 ` phcoder
2009-03-18 10:12 ` Robert Millan
2009-03-18 13:26 ` phcoder
2009-03-21 17:46 ` Robert Millan
2009-03-21 17:58 ` phcoder
2009-03-21 18:03 ` Robert Millan
2009-03-21 18:05 ` phcoder
2009-03-21 22:03 ` Robert Millan
2009-03-21 22:49 ` phcoder
2009-03-21 23:02 ` Robert Millan
2009-03-21 22:55 ` Robert Millan
2009-03-13 22:46 ` Robert Millan
2009-03-13 23:01 ` phcoder
2009-03-14 14:53 ` Robert Millan
2009-03-15 21:30 ` phcoder
[not found] <49B8F067.2040503@gmail.com>
[not found] ` <20090312.055819.95768237.davem@davemloft.net>
[not found] ` <49B90C69.60703@gmail.com>
[not found] ` <20090312.062628.260166400.davem@davemloft.net>
2009-03-12 13:43 ` phcoder
2009-03-12 14:05 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.