consolidate ELF header checks

consolidate ELF header checks

[Guillem Jover]

Hi,

I'm working on the kFreeBSD loader and thus merging some common
code to not have to duplicate it even more. This is the first patch
from this serie.

I've spoken with Marco about creating a new elf helper loader module
so other loaders can share common functions. Maybe later we can add
a.out or coff helper modules... Or maybe even move most of the
functionality from dl to the elf module.

This patch merges common ELF header checks. Please test this on PPC
as I've only tested on x86.


2005-02-01  Guillem Jover  <guillem@hadrons.org>

	* include/grub/dl.h (grub_dl_check_header): New prototype.
	(grub_arch_dl_check_header): Change return type to grub_err_t.
	* kern/dl.c (grub_dl_check_header): New function.
	(grub_dl_load_core): Use grub_dl_check_header instead of
	grub_arch_dl_check_header. Check ELF type. Check sections to be
	inside the core.
	* kern/i386/dl.c (grub_arch_dl_check_header): Remove arch
	independent ELF header checks.
	* kern/powerpc/dl.c (grub_arch_dl_check_header): Likewise.
	* loader/i386/pc/multiboot.c (grub_rescue_cmd_multiboot): Use
	grub_dl_check_header instead of explicit checks. Check for ELF type.
	* loader/powerpc/ieee1275/linux.c (grub_rescue_cmd_linux): Use
	grub_dl_check_header instead of explicit checks. Remove arch specific
	ELF header checks.
	* util/grub-emu.c (grub_arch_dl_check_header): Change return type.

diff -Naupr -x CVS grub2-cvs/include/grub/dl.h grub2.dl/include/grub/dl.h
--- grub2-cvs/include/grub/dl.h	2005-02-01 04:34:12.000000000 +0100
+++ grub2.dl/include/grub/dl.h	2005-02-01 04:03:26.000000000 +0100
@@ -70,6 +70,7 @@ struct grub_dl
 };
 typedef struct grub_dl *grub_dl_t;
 
+grub_err_t EXPORT_FUNC(grub_dl_check_header) (void *ehdr, grub_size_t size);
 grub_dl_t EXPORT_FUNC(grub_dl_load_file) (const char *filename);
 grub_dl_t EXPORT_FUNC(grub_dl_load) (const char *name);
 grub_dl_t grub_dl_load_core (void *addr, grub_size_t size);
@@ -84,7 +85,7 @@ grub_err_t EXPORT_FUNC(grub_dl_register_
 					    grub_dl_t mod);
 void *EXPORT_FUNC(grub_dl_resolve_symbol) (const char *name);
 
-int grub_arch_dl_check_header (void *ehdr, grub_size_t size);
+grub_err_t grub_arch_dl_check_header (void *ehdr, grub_size_t size);
 grub_err_t grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr);
 
 #endif /* ! GRUB_DL_H */
diff -Naupr -x CVS grub2-cvs/kern/dl.c grub2.dl/kern/dl.c
--- grub2-cvs/kern/dl.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/kern/dl.c	2005-02-01 04:05:45.000000000 +0100
@@ -238,6 +238,29 @@
   return 0;
 }
 
+/* Check if EHDR is a valid ELF header.  */
+grub_err_t
+grub_dl_check_header (void *ehdr, grub_size_t size)
+{
+  Elf_Ehdr *e = ehdr;
+
+  /* Check the header size.  */
+  if (size < sizeof (Elf_Ehdr))
+    return grub_error (GRUB_ERR_BAD_OS, "ELF size");
+
+  /* Check the magic numbers.  */
+  if (grub_arch_dl_check_header (ehdr, size)
+      || e->e_ident[EI_MAG0] != ELFMAG0
+      || e->e_ident[EI_MAG1] != ELFMAG1
+      || e->e_ident[EI_MAG2] != ELFMAG2
+      || e->e_ident[EI_MAG3] != ELFMAG3
+      || e->e_ident[EI_VERSION] != EV_CURRENT
+      || e->e_version != EV_CURRENT)
+    return grub_error (GRUB_ERR_BAD_OS, "Generic ELF magic");
+
+  return GRUB_ERR_NONE;
+}
+
 /* Load all segments from memory specified by E.  */
 static grub_err_t
 grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
@@ -497,12 +520,26 @@
   grub_dl_t mod;
   
   e = addr;
-  if (! grub_arch_dl_check_header (e, size))
+  if (grub_dl_check_header (e, size))
     {
       grub_error (GRUB_ERR_BAD_MODULE, "invalid ELF header");
       return 0;
     }
   
+  if (e->e_type != ET_REL)
+    {
+      grub_error (GRUB_ERR_BAD_MODULE,
+		  "This ELF file is not of the right type\n");
+      return 0;
+    }
+
+  /* Make sure that every section is within the core.  */
+  if (size < e->e_shoff + e->e_shentsize * e->e_shnum)
+    {
+      grub_error (GRUB_ERR_BAD_OS, "ELF sections inside core");
+      return 0;
+    }
+
   mod = (grub_dl_t) grub_malloc (sizeof (*mod));
   if (! mod)
     return 0;
diff -Naupr -x CVS grub2-cvs/kern/i386/dl.c grub2.dl/kern/i386/dl.c
--- grub2-cvs/kern/i386/dl.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/kern/i386/dl.c	2005-02-01 04:03:26.000000000 +0100
@@ -24,32 +24,18 @@
 #include <grub/err.h>
 
 /* Check if EHDR is a valid ELF header.  */
-int
+grub_err_t
 grub_arch_dl_check_header (void *ehdr, grub_size_t size)
 {
   Elf32_Ehdr *e = ehdr;
 
-  /* Check the header size.  */
-  if (size < sizeof (Elf32_Ehdr))
-    return 0;
-
   /* Check the magic numbers.  */
-  if (e->e_ident[EI_MAG0] != ELFMAG0
-      || e->e_ident[EI_MAG1] != ELFMAG1
-      || e->e_ident[EI_MAG2] != ELFMAG2
-      || e->e_ident[EI_MAG3] != ELFMAG3
-      || e->e_version != EV_CURRENT
-      || e->e_ident[EI_CLASS] != ELFCLASS32
+  if (e->e_ident[EI_CLASS] != ELFCLASS32
       || e->e_ident[EI_DATA] != ELFDATA2LSB
-      || e->e_machine != EM_386
-      || e->e_type != ET_REL)
-    return 0;
-
-  /* Make sure that every section is within the core.  */
-  if (size < e->e_shoff + e->e_shentsize * e->e_shnum)
-    return 0;
+      || e->e_machine != EM_386)
+    return grub_error (GRUB_ERR_BAD_OS, "Arch ELF magic");
 
-  return 1;
+  return GRUB_ERR_NONE;
 }
 
 /* Relocate symbols.  */
diff -Naupr -x CVS grub2-cvs/kern/powerpc/dl.c grub2.dl/kern/powerpc/dl.c
--- grub2-cvs/kern/powerpc/dl.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/kern/powerpc/dl.c	2005-02-01 04:03:26.000000000 +0100
@@ -24,32 +24,18 @@
 #include <grub/err.h>
 
 /* Check if EHDR is a valid ELF header.  */
-int
+grub_err_t
 grub_arch_dl_check_header (void *ehdr, grub_size_t size)
 {
   Elf32_Ehdr *e = ehdr;
 
-  /* Check the header size.  */
-  if (size < sizeof (Elf32_Ehdr))
-    return 0;
-
   /* Check the magic numbers.  */
-  if (!((e->e_ident[EI_MAG0] == ELFMAG0) 
-	&& (e->e_ident[EI_MAG1] == ELFMAG1)
-	&& (e->e_ident[EI_MAG2] == ELFMAG2) 
-	&& (e->e_ident[EI_MAG3] == ELFMAG3)
-	&& (e->e_ident[EI_CLASS] == ELFCLASS32) 
-	&& (e->e_ident[EI_DATA] == ELFDATA2MSB)
-	&& (e->e_ident[EI_VERSION] == EV_CURRENT) 
-	&& (e->e_type == ET_REL) && (e->e_machine == EM_PPC) 
-	&& (e->e_version == EV_CURRENT)))
-    return 0;
-  
-  /* Make sure that every section is within the core.  */
-  if (size < e->e_shoff + e->e_shentsize * e->e_shnum)
-    return 0;
+  if (e->e_ident[EI_CLASS] != ELFCLASS32
+      || e->e_ident[EI_DATA] != ELFDATA2MSB
+      || e->e_machine != EM_PPC)
+    return grub_error (GRUB_ERR_BAD_OS, "Arch ELF magic");
 
-  return 1;
+  return GRUB_ERR_NONE;
 }
 
 
diff -Naupr -x CVS grub2-cvs/loader/i386/pc/multiboot.c grub2.dl/loader/i386/pc/multiboot.c
--- grub2-cvs/loader/i386/pc/multiboot.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/loader/i386/pc/multiboot.c	2005-02-01 04:35:25.000000000 +0100
@@ -140,20 +140,19 @@ grub_rescue_cmd_multiboot (int argc, cha
 
   ehdr = (Elf32_Ehdr *) buffer;
 
-  if (!((ehdr->e_ident[EI_MAG0] == ELFMAG0) 
-	&& (ehdr->e_ident[EI_MAG1] == ELFMAG1)
-	&& (ehdr->e_ident[EI_MAG2] == ELFMAG2) 
-	&& (ehdr->e_ident[EI_MAG3] == ELFMAG3)
-	&& (ehdr->e_ident[EI_CLASS] == ELFCLASS32) 
-	&& (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
-	&& (ehdr->e_ident[EI_VERSION] == EV_CURRENT) 
-	&& (ehdr->e_type == ET_EXEC) && (ehdr->e_machine == EM_386) 
-	&& (ehdr->e_version == EV_CURRENT)))
+  if (grub_dl_check_header (ehdr, sizeof(*ehdr)))
     {
       grub_error (GRUB_ERR_UNKNOWN_OS, "No valid ELF header found");
       goto fail;
     }
 
+  if (ehdr->e_type != ET_EXEC)
+    {
+      grub_error (GRUB_ERR_UNKNOWN_OS,
+		  "This ELF file is not of the right type\n");
+      goto fail;
+    }
+
   /* FIXME: Should we support program headers at strange locations?  */
   if (ehdr->e_phoff + ehdr->e_phnum * ehdr->e_phentsize > GRUB_MB_SEARCH)
     {
diff -Naupr -x CVS grub2-cvs/loader/powerpc/ieee1275/linux.c grub2.dl/loader/powerpc/ieee1275/linux.c
--- grub2-cvs/loader/powerpc/ieee1275/linux.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/loader/powerpc/ieee1275/linux.c	2005-02-01 04:03:26.000000000 +0100
@@ -124,15 +124,7 @@ grub_rescue_cmd_linux (int argc, char *a
       goto fail;
     }
   
-  if (!((ehdr.e_ident[EI_MAG0] == ELFMAG0) 
-	&& (ehdr.e_ident[EI_MAG1] == ELFMAG1)
-	&& (ehdr.e_ident[EI_MAG2] == ELFMAG2) 
-	&& (ehdr.e_ident[EI_MAG3] == ELFMAG3)
-	&& (ehdr.e_ident[EI_CLASS] == ELFCLASS32) 
-	&& (ehdr.e_ident[EI_DATA] == ELFDATA2MSB)
-	&& (ehdr.e_ident[EI_VERSION] == EV_CURRENT) 
-	&& (ehdr.e_type == ET_EXEC) && (ehdr.e_machine == EM_PPC) 
-	&& (ehdr.e_version == EV_CURRENT)))
+  if (grub_dl_check_header (ehdr, sizeof(ehdr)))
     {
       grub_error (GRUB_ERR_UNKNOWN_OS, "No valid ELF header found");
       goto fail;
@@ -145,20 +137,6 @@ grub_rescue_cmd_linux (int argc, char *a
       goto fail;
     }
 
-  if (ehdr.e_machine != EM_PPC)
-    {
-      grub_error (GRUB_ERR_UNKNOWN_OS,
-		  "This ELF file is not for the PPC32\n");
-      goto fail;
-    }
-  
-  if (ehdr.e_version != EV_CURRENT)
-    {
-      grub_error (GRUB_ERR_UNKNOWN_OS,
-		  "Invalid ELF version\n");
-      goto fail;
-    }
-
   /* Read the sections.  */
   entry = ehdr.e_entry;
   if (entry == 0xc0000000)
diff -Naupr -x CVS grub2-cvs/util/grub-emu.c grub2.dl/util/grub-emu.c
--- grub2-cvs/util/grub-emu.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/util/grub-emu.c	2005-02-01 04:03:26.000000000 +0100
@@ -51,7 +51,7 @@ grub_arch_modules_addr (void)
   return 0;
 }
 
-int
+grub_err_t
 grub_arch_dl_check_header (void *ehdr, grub_size_t size)
 {
   (void) ehdr;

Re: consolidate ELF header checks

[Marco Gerards]

Guillem Jover <guillem@hadrons.org> writes:

> Hi,
>
> I'm working on the kFreeBSD loader and thus merging some common
> code to not have to duplicate it even more. This is the first patch
> from this serie.

That's really nice!  I am looking forward to your other patches.

> I've spoken with Marco about creating a new elf helper loader module
> so other loaders can share common functions. Maybe later we can add
> a.out or coff helper modules... Or maybe even move most of the
> functionality from dl to the elf module.

Yeah, I guess.  Let's see what is practical.

i don't think you can make a module of the code required by dl,
because dl is required to load it.  But it can be improved a lot at
least.

> This patch merges common ELF header checks. Please test this on PPC
> as I've only tested on x86.

I will test it.

> 2005-02-01  Guillem Jover  <guillem@hadrons.org>

The review of the patch will follow after I tested it.

Thanks,
Marco

Re: consolidate ELF header checks

[Marco Gerards]

Guillem Jover <guillem@hadrons.org> writes:

Hi,

> I've spoken with Marco about creating a new elf helper loader module
> so other loaders can share common functions. Maybe later we can add
> a.out or coff helper modules... Or maybe even move most of the
> functionality from dl to the elf module.
>
> This patch merges common ELF header checks. Please test this on PPC
> as I've only tested on x86.

I've done that.  I could load linux using this patch after a small
change.

> +  if (size < sizeof (Elf_Ehdr))
> +    return grub_error (GRUB_ERR_BAD_OS, "ELF size");

Please use a more descriptive error message.

> +    return grub_error (GRUB_ERR_BAD_OS, "Generic ELF magic");

Same here.

> -  /* Make sure that every section is within the core.  */
> -  if (size < e->e_shoff + e->e_shentsize * e->e_shnum)
> -    return 0;
> +      || e->e_machine != EM_386)
> +    return grub_error (GRUB_ERR_BAD_OS, "Arch ELF magic");

And here.

> diff -Naupr -x CVS grub2-cvs/loader/powerpc/ieee1275/linux.c grub2.dl/loader/powerpc/ieee1275/linux.c
> --- grub2-cvs/loader/powerpc/ieee1275/linux.c	2005-02-01 04:34:13.000000000 +0100
> +++ grub2.dl/loader/powerpc/ieee1275/linux.c	2005-02-01 04:03:26.000000000 +0100


> +  if (grub_dl_check_header (ehdr, sizeof(ehdr)))

The first ehdr should be &ehdr.

As far as I am concerned there are no other issues with this patch.
If someone else has problems with this patch, please tell me.
Otherwise I will commit it soon, if Guillem can fix these issues... :)

Thanks,
Marco

Re: consolidate ELF header checks

[Yoshinori K. Okuji]

On Friday 04 February 2005 23:25, Marco Gerards wrote:
> As far as I am concerned there are no other issues with this patch.
> If someone else has problems with this patch, please tell me.
> Otherwise I will commit it soon, if Guillem can fix these issues...
> :)

Has he already finished the legal matter?

Okuji

Re: consolidate ELF header checks

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Friday 04 February 2005 23:25, Marco Gerards wrote:
>> As far as I am concerned there are no other issues with this patch.
>> If someone else has problems with this patch, please tell me.
>> Otherwise I will commit it soon, if Guillem can fix these issues...
>> :)
>
> Has he already finished the legal matter?

Yes.

--
Marco

Re: consolidate ELF header checks

[Yoshinori K. Okuji]

On Saturday 05 February 2005 15:10, Marco Gerards wrote:
> > Has he already finished the legal matter?
>
> Yes.

It is strange, since I have never asked him to do that.

Guillem, who asked you to send a copyright assignment?

Okuji

Re: consolidate ELF header checks

[Guillem Jover]

Hi,

On Fri, Feb 04, 2005 at 10:25:03PM +0000, Marco Gerards wrote:
> Guillem Jover <guillem@hadrons.org> writes:
> > This patch merges common ELF header checks. Please test this on PPC
> > as I've only tested on x86.
> 
> I've done that.  I could load linux using this patch after a small
> change.

Thanks! :)

> As far as I am concerned there are no other issues with this patch.
> If someone else has problems with this patch, please tell me.
> Otherwise I will commit it soon, if Guillem can fix these issues... :)

I've fixed the error messages, the missing & and the unused size
variable warnings.

regards,
guillem


2005-02-05  Guillem Jover  <guillem@hadrons.org>

	* include/grub/dl.h (grub_dl_check_header): New prototype.
	(grub_arch_dl_check_header): Change return type to grub_err_t and
	remove size parameter. Fix all callers.
	* kern/dl.c (grub_dl_check_header): New function.
	(grub_dl_load_core): Use grub_dl_check_header instead of
	grub_arch_dl_check_header. Check ELF type. Check sections to be
	inside the core.
	* kern/i386/dl.c (grub_arch_dl_check_header): Remove arch
	independent ELF header checks.
	* kern/powerpc/dl.c (grub_arch_dl_check_header): Likewise.
	* loader/i386/pc/multiboot.c (grub_rescue_cmd_multiboot): Use
	grub_dl_check_header instead of explicit checks. Check for ELF type.
	* loader/powerpc/ieee1275/linux.c (grub_rescue_cmd_linux): Use
	grub_dl_check_header instead of explicit checks. Remove arch specific
	ELF header checks.

diff -Naupr -x CVS grub2-cvs/include/grub/dl.h grub2.dl/include/grub/dl.h
--- grub2-cvs/include/grub/dl.h	2005-02-01 04:34:12.000000000 +0100
+++ grub2.dl/include/grub/dl.h	2005-02-01 04:03:26.000000000 +0100
@@ -70,6 +70,7 @@ struct grub_dl
 };
 typedef struct grub_dl *grub_dl_t;
 
+grub_err_t EXPORT_FUNC(grub_dl_check_header) (void *ehdr, grub_size_t size);
 grub_dl_t EXPORT_FUNC(grub_dl_load_file) (const char *filename);
 grub_dl_t EXPORT_FUNC(grub_dl_load) (const char *name);
 grub_dl_t grub_dl_load_core (void *addr, grub_size_t size);
@@ -84,7 +85,7 @@ grub_err_t EXPORT_FUNC(grub_dl_register_
 					    grub_dl_t mod);
 void *EXPORT_FUNC(grub_dl_resolve_symbol) (const char *name);
 
-int grub_arch_dl_check_header (void *ehdr, grub_size_t size);
+grub_err_t grub_arch_dl_check_header (void *ehdr);
 grub_err_t grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr);
 
 #endif /* ! GRUB_DL_H */
diff -Naupr -x CVS grub2-cvs/kern/dl.c grub2.dl/kern/dl.c
--- grub2-cvs/kern/dl.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/kern/dl.c	2005-02-01 04:05:45.000000000 +0100
@@ -238,6 +238,29 @@
   return 0;
 }
 
+/* Check if EHDR is a valid ELF header.  */
+grub_err_t
+grub_dl_check_header (void *ehdr, grub_size_t size)
+{
+  Elf_Ehdr *e = ehdr;
+
+  /* Check the header size.  */
+  if (size < sizeof (Elf_Ehdr))
+    return grub_error (GRUB_ERR_BAD_OS, "ELF header smaller than expected");
+
+  /* Check the magic numbers.  */
+  if (grub_arch_dl_check_header (ehdr)
+      || e->e_ident[EI_MAG0] != ELFMAG0
+      || e->e_ident[EI_MAG1] != ELFMAG1
+      || e->e_ident[EI_MAG2] != ELFMAG2
+      || e->e_ident[EI_MAG3] != ELFMAG3
+      || e->e_ident[EI_VERSION] != EV_CURRENT
+      || e->e_version != EV_CURRENT)
+    return grub_error (GRUB_ERR_BAD_OS, "Invalid arch independent ELF magic");
+
+  return GRUB_ERR_NONE;
+}
+
 /* Load all segments from memory specified by E.  */
 static grub_err_t
 grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
@@ -497,12 +520,25 @@ grub_dl_load_core (void *addr, grub_size
   grub_dl_t mod;
   
   e = addr;
-  if (! grub_arch_dl_check_header (e, size))
+  if (grub_dl_check_header (e, size))
     {
       grub_error (GRUB_ERR_BAD_MODULE, "invalid ELF header");
       return 0;
     }
   
+  if (e->e_type != ET_REL)
+    {
+      grub_error (GRUB_ERR_BAD_MODULE, "Invalid ELF file type\n");
+      return 0;
+    }
+
+  /* Make sure that every section is within the core.  */
+  if (size < e->e_shoff + e->e_shentsize * e->e_shnum)
+    {
+      grub_error (GRUB_ERR_BAD_OS, "ELF sections outside core");
+      return 0;
+    }
+
   mod = (grub_dl_t) grub_malloc (sizeof (*mod));
   if (! mod)
     return 0;
diff -Naupr -x CVS grub2-cvs/kern/i386/dl.c grub2.dl/kern/i386/dl.c
--- grub2-cvs/kern/i386/dl.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/kern/i386/dl.c	2005-02-01 04:03:26.000000000 +0100
@@ -24,32 +24,18 @@
 #include <grub/err.h>
 
 /* Check if EHDR is a valid ELF header.  */
-int
-grub_arch_dl_check_header (void *ehdr, grub_size_t size)
+grub_err_t
+grub_arch_dl_check_header (void *ehdr)
 {
   Elf32_Ehdr *e = ehdr;
 
-  /* Check the header size.  */
-  if (size < sizeof (Elf32_Ehdr))
-    return 0;
-
   /* Check the magic numbers.  */
-  if (e->e_ident[EI_MAG0] != ELFMAG0
-      || e->e_ident[EI_MAG1] != ELFMAG1
-      || e->e_ident[EI_MAG2] != ELFMAG2
-      || e->e_ident[EI_MAG3] != ELFMAG3
-      || e->e_version != EV_CURRENT
-      || e->e_ident[EI_CLASS] != ELFCLASS32
+  if (e->e_ident[EI_CLASS] != ELFCLASS32
       || e->e_ident[EI_DATA] != ELFDATA2LSB
-      || e->e_machine != EM_386
-      || e->e_type != ET_REL)
-    return 0;
-
-  /* Make sure that every section is within the core.  */
-  if (size < e->e_shoff + e->e_shentsize * e->e_shnum)
-    return 0;
+      || e->e_machine != EM_386)
+    return grub_error (GRUB_ERR_BAD_OS, "Invalid arch specific ELF magic");
 
-  return 1;
+  return GRUB_ERR_NONE;
 }
 
 /* Relocate symbols.  */
diff -Naupr -x CVS grub2-cvs/kern/powerpc/dl.c grub2.dl/kern/powerpc/dl.c
--- grub2-cvs/kern/powerpc/dl.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/kern/powerpc/dl.c	2005-02-01 04:03:26.000000000 +0100
@@ -24,32 +24,18 @@
 #include <grub/err.h>
 
 /* Check if EHDR is a valid ELF header.  */
-int
-grub_arch_dl_check_header (void *ehdr, grub_size_t size)
+grub_err_t
+grub_arch_dl_check_header (void *ehdr)
 {
   Elf32_Ehdr *e = ehdr;
 
-  /* Check the header size.  */
-  if (size < sizeof (Elf32_Ehdr))
-    return 0;
-
   /* Check the magic numbers.  */
-  if (!((e->e_ident[EI_MAG0] == ELFMAG0) 
-	&& (e->e_ident[EI_MAG1] == ELFMAG1)
-	&& (e->e_ident[EI_MAG2] == ELFMAG2) 
-	&& (e->e_ident[EI_MAG3] == ELFMAG3)
-	&& (e->e_ident[EI_CLASS] == ELFCLASS32) 
-	&& (e->e_ident[EI_DATA] == ELFDATA2MSB)
-	&& (e->e_ident[EI_VERSION] == EV_CURRENT) 
-	&& (e->e_type == ET_REL) && (e->e_machine == EM_PPC) 
-	&& (e->e_version == EV_CURRENT)))
-    return 0;
-  
-  /* Make sure that every section is within the core.  */
-  if (size < e->e_shoff + e->e_shentsize * e->e_shnum)
-    return 0;
+  if (e->e_ident[EI_CLASS] != ELFCLASS32
+      || e->e_ident[EI_DATA] != ELFDATA2MSB
+      || e->e_machine != EM_PPC)
+    return grub_error (GRUB_ERR_BAD_OS, "Invalid arch specific ELF magic");
 
-  return 1;
+  return GRUB_ERR_NONE;
 }
 
 
diff -Naupr -x CVS grub2-cvs/loader/i386/pc/multiboot.c grub2.dl/loader/i386/pc/multiboot.c
--- grub2-cvs/loader/i386/pc/multiboot.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/loader/i386/pc/multiboot.c	2005-02-01 04:35:25.000000000 +0100
@@ -140,20 +140,18 @@ grub_rescue_cmd_multiboot (int argc, cha
 
   ehdr = (Elf32_Ehdr *) buffer;
 
-  if (!((ehdr->e_ident[EI_MAG0] == ELFMAG0) 
-	&& (ehdr->e_ident[EI_MAG1] == ELFMAG1)
-	&& (ehdr->e_ident[EI_MAG2] == ELFMAG2) 
-	&& (ehdr->e_ident[EI_MAG3] == ELFMAG3)
-	&& (ehdr->e_ident[EI_CLASS] == ELFCLASS32) 
-	&& (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
-	&& (ehdr->e_ident[EI_VERSION] == EV_CURRENT) 
-	&& (ehdr->e_type == ET_EXEC) && (ehdr->e_machine == EM_386) 
-	&& (ehdr->e_version == EV_CURRENT)))
+  if (grub_dl_check_header (ehdr, sizeof(*ehdr)))
     {
       grub_error (GRUB_ERR_UNKNOWN_OS, "No valid ELF header found");
       goto fail;
     }
 
+  if (ehdr->e_type != ET_EXEC)
+    {
+      grub_error (GRUB_ERR_UNKNOWN_OS, "Invalid ELF file type\n");
+      goto fail;
+    }
+
   /* FIXME: Should we support program headers at strange locations?  */
   if (ehdr->e_phoff + ehdr->e_phnum * ehdr->e_phentsize > GRUB_MB_SEARCH)
     {
diff -Naupr -x CVS grub2-cvs/loader/powerpc/ieee1275/linux.c grub2.dl/loader/powerpc/ieee1275/linux.c
--- grub2-cvs/loader/powerpc/ieee1275/linux.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/loader/powerpc/ieee1275/linux.c	2005-02-01 04:03:26.000000000 +0100
@@ -124,15 +124,7 @@ grub_rescue_cmd_linux (int argc, char *a
       goto fail;
     }
   
-  if (!((ehdr.e_ident[EI_MAG0] == ELFMAG0) 
-	&& (ehdr.e_ident[EI_MAG1] == ELFMAG1)
-	&& (ehdr.e_ident[EI_MAG2] == ELFMAG2) 
-	&& (ehdr.e_ident[EI_MAG3] == ELFMAG3)
-	&& (ehdr.e_ident[EI_CLASS] == ELFCLASS32) 
-	&& (ehdr.e_ident[EI_DATA] == ELFDATA2MSB)
-	&& (ehdr.e_ident[EI_VERSION] == EV_CURRENT) 
-	&& (ehdr.e_type == ET_EXEC) && (ehdr.e_machine == EM_PPC) 
-	&& (ehdr.e_version == EV_CURRENT)))
+  if (grub_dl_check_header (&ehdr, sizeof(ehdr)))
     {
       grub_error (GRUB_ERR_UNKNOWN_OS, "No valid ELF header found");
       goto fail;
@@ -145,20 +137,6 @@ grub_rescue_cmd_linux (int argc, char *a
       goto fail;
     }
 
-  if (ehdr.e_machine != EM_PPC)
-    {
-      grub_error (GRUB_ERR_UNKNOWN_OS,
-		  "This ELF file is not for the PPC32\n");
-      goto fail;
-    }
-  
-  if (ehdr.e_version != EV_CURRENT)
-    {
-      grub_error (GRUB_ERR_UNKNOWN_OS,
-		  "Invalid ELF version\n");
-      goto fail;
-    }
-
   /* Read the sections.  */
   entry = ehdr.e_entry;
   if (entry == 0xc0000000)
diff -Naupr -x CVS grub2-cvs/util/grub-emu.c grub2.dl/util/grub-emu.c
--- grub2-cvs/util/grub-emu.c	2005-02-01 04:34:13.000000000 +0100
+++ grub2.dl/util/grub-emu.c	2005-02-01 04:03:26.000000000 +0100
@@ -51,11 +51,10 @@ grub_arch_modules_addr (void)
   return 0;
 }
 
-int
-grub_arch_dl_check_header (void *ehdr, grub_size_t size)
+grub_err_t
+grub_arch_dl_check_header (void *ehdr)
 {
   (void) ehdr;
-  (void) size;
 
   return GRUB_ERR_BAD_MODULE;
 }

Re: consolidate ELF header checks

[Marco Gerards]

Guillem Jover <guillem@hadrons.org> writes:

>> As far as I am concerned there are no other issues with this patch.
>> If someone else has problems with this patch, please tell me.
>> Otherwise I will commit it soon, if Guillem can fix these issues... :)
>
> I've fixed the error messages, the missing & and the unused size
> variable warnings.

Great!

Did you fix that warning I told you about on IRC?  Otherwise I'll do
that before I commit the patch.  Same for a few small things that need
to be fixed.

If there are no problems with this patch I will commit it on Tuesday.

Thanks,
Marco

Re: consolidate ELF header checks

[Yoshinori K. Okuji]

On Sunday 06 February 2005 12:02, Marco Gerards wrote:
> If there are no problems with this patch I will commit it on Tuesday.

Don't do that. I must make sure that his copyright assignment has been 
done correctly. I don't know why, but he seems not to want to reply to 
my question. It irritates me a lot.

Okuji

Re: consolidate ELF header checks

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Sunday 06 February 2005 12:02, Marco Gerards wrote:
>> If there are no problems with this patch I will commit it on Tuesday.
>
> Don't do that. I must make sure that his copyright assignment has been 
> done correctly. I don't know why, but he seems not to want to reply to 
> my question. It irritates me a lot.

I've checked it and his copyrights are assigned.  It looks like he
contacted assign@gnu.org on his own.  AFAIK that is sometimes a normal
procedure, for example when you are contributing to a lot of GNU
projects.

Anyway, I will wait until Guillem replies to your email.

--
Marco

Re: consolidate ELF header checks

[Guillem Jover]

Hi Okuji,

On Sun, Feb 06, 2005 at 01:58:26PM +0100, Yoshinori K. Okuji wrote:
> Don't do that. I must make sure that his copyright assignment has been 
> done correctly. I don't know why, but he seems not to want to reply to 
> my question. It irritates me a lot.

Sorry about that, it was not intentional. You asked Marco and he
replied so I though that was enough, then I sent the patch revision
Marco asked and left home, didn't saw your other mails until today.

On another thread, Yoshinori K. Okuji wrote:
> It is strange, since I have never asked him to do that.
>
> Guillem, who asked you to send a copyright assignment?

I was hacking on the kFreeBSD loader for GRUB Legacy and needed
other copyright assignments for other GNU projects, so I asked for
GRUB as well. Marco or Jeff pointed me to the assignment procudures.

Hope all is ok now, if you need more info just ask.

regards,
guillem

Re: consolidate ELF header checks

[Yoshinori K. Okuji]

On Sunday 06 February 2005 22:28, Guillem Jover wrote:
> Sorry about that, it was not intentional. You asked Marco and he
> replied so I though that was enough, then I sent the patch revision
> Marco asked and left home, didn't saw your other mails until today.

It's okay. Don't mind. I just want to make things clear.

> I was hacking on the kFreeBSD loader for GRUB Legacy and needed
> other copyright assignments for other GNU projects, so I asked for
> GRUB as well. Marco or Jeff pointed me to the assignment procudures.

I need to know how you did precisely. Did you ask assign@gnu.org or 
something else? What template did you use? Did any maintainer of GNU 
Software specify how to proceed it?


BTW, please contact appropriate maintainers before trying to assign 
copyright from now on. Maintainers of GNU Software are responsible for 
managing copyright issues. If you bypass the maintainers, they may not 
keep things consistent, since they are not notified even if the 
procedures are wrong.

For example, I am very afraid that you might have used an obsolete 
request form. The FSF revises the request form and even the whole 
procedures from time to time, so you need to request up-to-date 
information when you assign copyright. 

Another - more serious - case is that you might have ignored the first 
request completely and simply sent a filled assignment paper to the 
FSF. You could do this, if you get a copy from someone who has assigned 
copyright. If this happens, you wouldn't be informed of all the 
cautions by the FSF, such as a copyright disclaimer by your employer.

Okuji

Re: consolidate ELF header checks

[Guillem Jover]

On Tue, Feb 08, 2005 at 01:54:01PM +0100, Yoshinori K. Okuji wrote:
> On Sunday 06 February 2005 22:28, Guillem Jover wrote:
> > I was hacking on the kFreeBSD loader for GRUB Legacy and needed
> > other copyright assignments for other GNU projects, so I asked for
> > GRUB as well. Marco or Jeff pointed me to the assignment procudures.
> 
> I need to know how you did precisely. Did you ask assign@gnu.org or 
> something else? What template did you use? Did any maintainer of GNU 
> Software specify how to proceed it?

I just sent a mail to assign@gnu.org and followed the instructions
and used the templates received.

> BTW, please contact appropriate maintainers before trying to assign 
> copyright from now on. Maintainers of GNU Software are responsible for 
> managing copyright issues. If you bypass the maintainers, they may not 
> keep things consistent, since they are not notified even if the 
> procedures are wrong.

Ok I will.

> Another - more serious - case is that you might have ignored the first 
> request completely and simply sent a filled assignment paper to the 
> FSF. You could do this, if you get a copy from someone who has assigned 
> copyright. If this happens, you wouldn't be informed of all the 
> cautions by the FSF, such as a copyright disclaimer by your employer.

I got informed of those, and any of such cautions applied in my case.

Is this all ok now, or do you need more info?

regards,
guillem

Re: consolidate ELF header checks

[Yoshinori K. Okuji]

On Wednesday 09 February 2005 08:10, Guillem Jover wrote:
> I just sent a mail to assign@gnu.org and followed the instructions
> and used the templates received.

I see.

Okuji

Re: consolidate ELF header checks

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Wednesday 09 February 2005 08:10, Guillem Jover wrote:
>> I just sent a mail to assign@gnu.org and followed the instructions
>> and used the templates received.
>
> I see.

I assume that means I can commit the patch.  I'll do that on Sunday
evening.  Just tell me if there are still problems and I will wait
with committing the patch.

Thanks,
Marco

Re: consolidate ELF header checks

[Yoshinori K. Okuji]

On Friday 11 February 2005 18:25, Marco Gerards wrote:
> I assume that means I can commit the patch.  I'll do that on Sunday
> evening.  Just tell me if there are still problems and I will wait
> with committing the patch.

One minor problem in his patch is this part:

@@ -497,12 +520,25 @@ grub_dl_load_core (void *addr, grub_size
   grub_dl_t mod;
   
   e = addr;
-  if (! grub_arch_dl_check_header (e, size))
+  if (grub_dl_check_header (e, size))
     {
       grub_error (GRUB_ERR_BAD_MODULE, "invalid ELF header");
       return 0;
     }
   
As grub_dl_check_header is supposed to provide a more detailed error, 
grub_dl_load_core should not set a new error here.

Once this is fixed, I have no objection.

Okuji

Re: consolidate ELF header checks

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Friday 11 February 2005 18:25, Marco Gerards wrote:
>> I assume that means I can commit the patch.  I'll do that on Sunday
>> evening.  Just tell me if there are still problems and I will wait
>> with committing the patch.
>
> One minor problem in his patch is this part:

[...]

> Once this is fixed, I have no objection.

Ok.  This will be done when I commit the patch.

Thanks,
Marco