[PATCH] ELF program header

[PATCH] ELF program header

[Robert Millan]

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


It seems that grub-mkimage generates awkward ELF files, in which the Program
header table is at the end of the file instead of right after the ELF header.

I know very little about ELF, but:

  - This figure in ELF standard seems to indicate which is the "normal" (not
  sure if mandatory) location:

    http://www.cs.ucdavis.edu/~haungs/paper/node11.html

  - Our own ELF loader doesn't like phdroff > 0x2000 either, from
    loader/i386/pc/multiboot.c:

  /* FIXME: Should we support program headers at strange locations?  */
  if (ehdr->e_phoff + ehdr->e_phnum * ehdr->e_phentsize > MULTIBOOT_SEARCH)
    return grub_error (GRUB_ERR_BAD_OS, "program header at a too high offset");

This breaks self-boot in the LinuxBIOS target.  Moving the Program header
(see attached patch) fixed it, with no apparent drawbacks or regressions in
any of the ELF loaders around (tested on LinuxBIOS ELF loader and Efika OF).

I'm not completely sure of its correctness though, and would appreciate if
someone with a better understanding of ELF can comment on it.  In particular,
I don't know if my proposed solution could overwrite valid data.  Are the
segments garanteed to always leave room for the program header, do we
have to explicitly check for that, or perhaps we need to relocate the segments
when needed?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

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

2007-10-12  Robert Millan  <rmh@aybabtu.com>

	* util/elf/grub-mkimage.c (add_segments): Allocate Program header
	table right after ELF header.

diff -ur grub2/util/elf/grub-mkimage.c grub2.phdr/util/elf/grub-mkimage.c
--- grub2/util/elf/grub-mkimage.c	2007-10-12 12:22:27.000000000 +0200
+++ grub2.phdr/util/elf/grub-mkimage.c	2007-10-12 12:36:38.000000000 +0200
@@ -250,7 +250,7 @@
   ehdr.e_shstrndx = 0;
 
   /* Append entire segment table to the file.  */
-  phdroff = ALIGN_UP (grub_util_get_fp_size (out), sizeof (long));
+  phdroff = ALIGN_UP (sizeof (ehdr), sizeof (long));
   grub_util_write_image_at (phdrs, grub_target_to_host16 (ehdr.e_phentsize)
 			    * grub_target_to_host16 (ehdr.e_phnum), phdroff,
 			    out);

Re: [PATCH] ELF program header

[Andrei E. Warkentin]

12.10.2007, в 11:19, Robert Millan писал(а):

>
> It seems that grub-mkimage generates awkward ELF files, in which  
> the Program
> header table is at the end of the file instead of right after the  
> ELF header.
>
> I know very little about ELF, but:
>
>   - This figure in ELF standard seems to indicate which is the  
> "normal" (not
>   sure if mandatory) location:
>
>     http://www.cs.ucdavis.edu/~haungs/paper/node11.html
>
>   - Our own ELF loader doesn't like phdroff > 0x2000 either, from
>     loader/i386/pc/multiboot.c:
>
>   /* FIXME: Should we support program headers at strange  
> locations?  */
>   if (ehdr->e_phoff + ehdr->e_phnum * ehdr->e_phentsize >  
> MULTIBOOT_SEARCH)
>     return grub_error (GRUB_ERR_BAD_OS, "program header at a too  
> high offset");
>
> This breaks self-boot in the LinuxBIOS target.  Moving the Program  
> header
> (see attached patch) fixed it, with no apparent drawbacks or  
> regressions in
> any of the ELF loaders around (tested on LinuxBIOS ELF loader and  
> Efika OF).
>
> I'm not completely sure of its correctness though, and would  
> appreciate if
> someone with a better understanding of ELF can comment on it.  In  
> particular,
> I don't know if my proposed solution could overwrite valid data.   
> Are the
> segments garanteed to always leave room for the program header, do we
> have to explicitly check for that, or perhaps we need to relocate  
> the segments
> when needed?

I don't think the ELF TIS says anything about location of PHDRs.  
That's the whole point of EHDR - so that the location, size of each  
entry, and count can be variable. The only invalid case would be for  
phoff or phoff + phentcount * phentsize to exceed end of file offset.

>
> -- 
> Robert Millan
>
> <GPLv2> I know my rights; I want my phone call!
> <DRM> What use is a phone call, if you are unable to speak?
> (as seen on /.)
> <phdr.diff>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] ELF program header

[Stefan Reinauer]

* Robert Millan <rmh@aybabtu.com> [071012 18:19]:
> It seems that grub-mkimage generates awkward ELF files, in which the Program
> header table is at the end of the file instead of right after the ELF header.
> 
> 
>   - Our own ELF loader doesn't like phdroff > 0x2000 either, from
>     loader/i386/pc/multiboot.c:

I think such a fixed address makes little sense, however there are
reasons to put the phdr in front.

>   /* FIXME: Should we support program headers at strange locations?  */
>   if (ehdr->e_phoff + ehdr->e_phnum * ehdr->e_phentsize > MULTIBOOT_SEARCH)
>     return grub_error (GRUB_ERR_BAD_OS, "program header at a too high offset");
> 
> This breaks self-boot in the LinuxBIOS target.  Moving the Program header
> (see attached patch) fixed it, with no apparent drawbacks or regressions in
> any of the ELF loaders around (tested on LinuxBIOS ELF loader and Efika OF).

I assume this was with LinuxBIOSv3 and Qemu?

> I'm not completely sure of its correctness though, and would appreciate if
> someone with a better understanding of ELF can comment on it.  

There's one good reason to put ELF headers in the beginning, that is
streaming of ELF files. If you get your ELF from a streaming
decompression algorithm you have to 
- either copy your elf to memory completely before loading it a second
  time
- or decompress it twice
- or you put the phdr in the beginning :-)

> I don't know if my proposed solution could overwrite valid data.  Are the
> segments garanteed to always leave room for the program header, do we
> have to explicitly check for that, or perhaps we need to relocate the segments
> when needed?

We tried the same but the patch was a lot bigger, trying to actually
leave space in the beginning. Your patch might be enough, that would be
really nice. I didn't really get the sense behind the layers of grub
magic around read() and write(). Looking at the code, it looks
completely wrong, but it surprisingly seemed to work in LBv3.

>    /* Append entire segment table to the file.  */
> -  phdroff = ALIGN_UP (grub_util_get_fp_size (out), sizeof (long));
> +  phdroff = ALIGN_UP (sizeof (ehdr), sizeof (long));
>    grub_util_write_image_at (phdrs, grub_target_to_host16 (ehdr.e_phentsize)
>  			    * grub_target_to_host16 (ehdr.e_phnum), phdroff,
>  			    out);

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info@coresystems.de  • http://www.coresystems.de/

Re: [PATCH] ELF program header

[Robert Millan]

On Fri, Oct 12, 2007 at 03:24:06PM -0500, Andrei E. Warkentin wrote:
> 
> I don't think the ELF TIS says anything about location of PHDRs.  
> That's the whole point of EHDR - so that the location, size of each  
> entry, and count can be variable. The only invalid case would be for  
> phoff or phoff + phentcount * phentsize to exceed end of file offset.

But what do we know about the segments?  Could we be accidentaly overwriting
the first segment with our phdr?  It didn't happen in any of my tests, but
I'm not sure if it's still possible.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Re: [PATCH] ELF program header

[Robert Millan]

On Sat, Oct 13, 2007 at 12:03:37AM +0200, Stefan Reinauer wrote:
> >   /* FIXME: Should we support program headers at strange locations?  */
> >   if (ehdr->e_phoff + ehdr->e_phnum * ehdr->e_phentsize > MULTIBOOT_SEARCH)
> >     return grub_error (GRUB_ERR_BAD_OS, "program header at a too high offset");
> > 
> > This breaks self-boot in the LinuxBIOS target.  Moving the Program header
> > (see attached patch) fixed it, with no apparent drawbacks or regressions in
> > any of the ELF loaders around (tested on LinuxBIOS ELF loader and Efika OF).
> 
> I assume this was with LinuxBIOSv3 and Qemu?

Yes.

> We tried the same but the patch was a lot bigger, trying to actually
> leave space in the beginning. Your patch might be enough, that would be
> really nice.

That's the question.  Do we really have to add more space, or is that space
already allocated for us?

> I didn't really get the sense behind the layers of grub
> magic around read() and write().

They're basicaly like pread() and pwrite() with the added functionality of a
verbose mode.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Re: [PATCH] ELF program header

[Marcin Kurek]

Hell[o]

> [...]

I test this patch on Pegasos 1 and Pegasos 2 and in both cases grub
image hangs on OF prompt when I tired to boot it :( Reverting it makes
grub bootable again.

-- 
--- Marcin 'Morgoth' Kurek ---

Re: [PATCH] ELF program header

[Stefan Reinauer]

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

* Robert Millan <rmh@aybabtu.com> [071014 14:00]:
> > We tried the same but the patch was a lot bigger, trying to actually
> > leave space in the beginning. Your patch might be enough, that would be
> > really nice.
> 
> That's the question.  Do we really have to add more space, or is that space
> already allocated for us?

Ok, I am not exactly sure how to correctly adapt to the latest
grub-mkimage changes, so please bear that the attached patch is against
Patrick's GSoC version of grub-mkimage.c

With this patch attached, grub-mkimage produces an image that works when
being loaded as payload from LinuxBIOS version 2.

Your smaller version of the patch unfortunately did not work for me.

Flames and suggestions please! 

Best wishes, 

Stefan



[-- Attachment #2: grub-mkimage-working.diff --]
[-- Type: text/x-patch, Size: 2387 bytes --]

--- grub-mkimage.c.orig	2007-10-05 20:33:50.000000000 +0000
+++ grub-mkimage.c	2007-10-15 18:20:03.000000000 +0000
@@ -104,7 +104,7 @@
   FILE *in;
   char *kernel_path;
   grub_addr_t grub_end = 0;
-  off_t phdroff;
+  off_t phdroff, pstart;
   int i;
 
   /* Read ELF header.  */
@@ -115,18 +115,27 @@
 
   grub_util_read_at (&ehdr, sizeof (ehdr), 0, in);
   
+  /* The +1 is to leave a hole for the program header for extra modules. */
   phdrs = xmalloc (grub_le_to_cpu16 (ehdr.e_phentsize)
-		   * (grub_le_to_cpu16 (ehdr.e_phnum) + 2));
+		   * (grub_le_to_cpu16 (ehdr.e_phnum) + 1));
+  
+  /* Append entire segment table to the file.  */
+  //phdroff = ALIGN_UP (grub_util_get_fp_size (out), sizeof (long));
+  phdroff = ALIGN_UP (sizeof(ehdr), sizeof (long));
+  grub_util_write_image_at (phdrs, grub_le_to_cpu16 (ehdr.e_phentsize)
+			    *(grub_le_to_cpu16 (ehdr.e_phnum) + 1), phdroff,
+			    out);
+
   /* Copy all existing segments.  */
   grub_util_info ("%u segments", grub_le_to_cpu16 (ehdr.e_phnum));
+  pstart = grub_util_get_fp_size (out);
+  /* set up first phdr offset */
   for (i = 0; i < grub_le_to_cpu16 (ehdr.e_phnum); i++)
     {
       char *segment_img;
       grub_size_t segment_end;
-
       phdr = phdrs + i;
-
-      /* Read segment header.  */
+     /* Read segment header.  */
       grub_util_read_at (phdr, sizeof (Elf32_Phdr),
 			 (grub_le_to_cpu32 (ehdr.e_phoff)
 			  + (i * grub_le_to_cpu16 (ehdr.e_phentsize))),
@@ -146,9 +155,11 @@
   
       grub_util_read_at (segment_img, grub_le_to_cpu32 (phdr->p_filesz),
 			 grub_le_to_cpu32 (phdr->p_offset), in);
-      grub_util_write_image_at (segment_img, grub_le_to_cpu32 (phdr->p_filesz),
+     phdr->p_offset = grub_cpu_to_le32(pstart);
+     grub_util_write_image_at (segment_img, grub_le_to_cpu32 (phdr->p_filesz),
 				grub_le_to_cpu32 (phdr->p_offset), out);
 
+      pstart += phdr->p_filesz;
       free (segment_img);
     }
 
@@ -176,9 +187,9 @@
   ehdr.e_shstrndx = 0;
 
   /* Append entire segment table to the file.  */
-  phdroff = ALIGN_UP (grub_util_get_fp_size (out), sizeof (long));
+  // phdroff = ALIGN_UP (grub_util_get_fp_size (out), sizeof (long));
   grub_util_write_image_at (phdrs, grub_le_to_cpu16 (ehdr.e_phentsize)
-			    * grub_le_to_cpu16 (ehdr.e_phnum), phdroff,
+			    * (grub_le_to_cpu16 (ehdr.e_phnum)), phdroff,
 			    out);
 
   /* Write ELF header.  */