[PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE)

[PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE)

[Christian Franke]

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

HAVE_ASM_USCORE is checked by configure, but module loader does not work 
in this case.

This patch fixes this by ignoring the difference. It actually works to 
load modules compiled on Linux by a kernel compiled on Cygwin (with 
underscores) and vice versa.

It also prepares the grub_mod_init/finit search for modules converted 
from non-ELF format (which do not support the "function" symbol type).

A bug in symbol table scan is fixed.

Christian

2007-11-10  Christian Franke  <franke@computer.org>

	* genkernsyms.sh.in: Handle HAVE_ASM_USCORE case.

	* genmk.rb: Handle HAVE_ASM_USCORE case in strip command.
	Add $EXEEXT to CLEANFILES.

	* kern/dl.c (grub_dl_resolve_symbol): Handle HAVE_ASM_USCORE case,
	ignore leading underscore.
	(grub_dl_register_symbol): Likewise.
	(grub_dl_resolve_symbols): Likewise.
	Add check for grub_mod_init/fini for symbols without type. 
	(grub_dl_resolve_dependencies): Add check for trailing nullbytes
	in symbol table. This fixes an infinite loop if stab is zero filled.



[-- Attachment #2: grub2-asm_uscore.patch --]
[-- Type: text/x-patch, Size: 4636 bytes --]

diff -rup grub2.orig/genkernsyms.sh.in grub2/genkernsyms.sh.in
--- grub2.orig/genkernsyms.sh.in	2006-07-12 22:42:52.000000000 +0200
+++ grub2/genkernsyms.sh.in	2007-10-06 12:59:29.000000000 +0200
@@ -16,9 +16,12 @@
 srcdir=@srcdir@
 CC="@CC@"
 
+u=
+grep "^#define HAVE_ASM_USCORE" config.h >/dev/null 2>&1 && u="_"
+
 $CC -DGRUB_SYMBOL_GENERATOR=1 -E -I. -Iinclude -I$srcdir/include $* \
   | grep -v '^#' \
   | sed -n \
-        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/\1 kernel/;p;}' \
-        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/\1 kernel/;p;}' \
+        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
+        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
   | sort -u
diff -rup grub2.orig/genmk.rb grub2/genmk.rb
--- grub2.orig/genmk.rb	2007-10-20 22:49:38.968750000 +0200
+++ grub2/genmk.rb	2007-11-10 19:37:17.562500000 +0100
@@ -115,7 +115,7 @@ UNDSYMFILES += #{undsym}
 #{@name}: #{pre_obj} #{mod_obj}
 	-rm -f $@
 	$(TARGET_CC) $(#{prefix}_LDFLAGS) $(TARGET_LDFLAGS) -Wl,-r,-d -o $@ $^
-	$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -R .note -R .comment $@
+	$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -K _grub_mod_init -K _grub_mod_fini -R .note -R .comment $@
 
 #{pre_obj}: $(#{prefix}_DEPENDENCIES) #{objs_str}
 	-rm -f $@
@@ -187,7 +187,7 @@ class Utility
     deps = objs.collect {|obj| obj.suffix('d')}
     deps_str = deps.join(' ');
 
-    "CLEANFILES += #{@name} #{objs_str}
+    "CLEANFILES += #{@name}$(EXEEXT) #{objs_str}
 MOSTLYCLEANFILES += #{deps_str}
 
 #{@name}: $(#{prefix}_DEPENDENCIES) #{objs_str}
diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c
--- grub2.orig/kern/dl.c	2007-07-22 01:32:26.000000000 +0200
+++ grub2/kern/dl.c	2007-11-10 19:44:34.140625000 +0100
@@ -156,6 +156,9 @@ grub_dl_resolve_symbol (const char *name
 {
   grub_symbol_t sym;
 
+  if (*name == '_')
+    name++;
+
   for (sym = grub_symtab[grub_symbol_hash (name)]; sym; sym = sym->next)
     if (grub_strcmp (sym->name, name) == 0)
       return sym->addr;
@@ -169,6 +172,11 @@ grub_dl_register_symbol (const char *nam
 {
   grub_symbol_t sym;
   unsigned k;
+
+  /* Ignore leading underscore used for C symbols.
+     Done at runtime to allow loading modules compiled on other OS.  */
+  if (*name == '_')
+    name++;
   
   sym = (grub_symbol_t) grub_malloc (sizeof (*sym));
   if (! sym)
@@ -347,6 +355,7 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
       unsigned char type = ELF_ST_TYPE (sym->st_info);
       unsigned char bind = ELF_ST_BIND (sym->st_info);
       const char *name = str + sym->st_name;
+      int check_mod_func = 0;
       
       switch (type)
 	{
@@ -359,6 +368,12 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 		return grub_error (GRUB_ERR_BAD_MODULE,
 				   "the symbol `%s' not found", name);
 	    }
+	  else if (sym->st_name != 0 && bind == STB_LOCAL)
+	    { /* Static functions have no type if initial format was not ELF.  */
+	      sym->st_value += (Elf_Addr) grub_dl_get_section_addr (mod,
+								    sym->st_shndx);
+	      check_mod_func = 1;
+	    }
 	  else
 	    sym->st_value = 0;
 	  break;
@@ -374,14 +389,11 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 	case STT_FUNC:
 	  sym->st_value += (Elf_Addr) grub_dl_get_section_addr (mod,
 								sym->st_shndx);
-	  if (bind != STB_LOCAL)
+	  if (bind == STB_LOCAL)
+	    check_mod_func = 1;
+	  else	
 	    if (grub_dl_register_symbol (name, (void *) sym->st_value, mod))
 	      return grub_errno;
-	  
-	  if (grub_strcmp (name, "grub_mod_init") == 0)
-	    mod->init = (void (*) (grub_dl_t)) sym->st_value;
-	  else if (grub_strcmp (name, "grub_mod_fini") == 0)
-	    mod->fini = (void (*) (void)) sym->st_value;
 	  break;
 
 	case STT_SECTION:
@@ -397,6 +409,15 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 	  return grub_error (GRUB_ERR_BAD_MODULE,
 			     "unknown symbol type `%d'", (int) type);
 	}
+      if (check_mod_func)
+        {
+	  if (*name == '_')
+	    name++;
+	  if (grub_strcmp (name, "grub_mod_init") == 0)
+	    mod->init = (void (*) (grub_dl_t)) sym->st_value;
+	  else if (grub_strcmp (name, "grub_mod_fini") == 0)
+	    mod->fini = (void (*) (void)) sym->st_value;
+        }
     }
 
   return GRUB_ERR_NONE;
@@ -454,7 +475,7 @@ grub_dl_resolve_dependencies (grub_dl_t 
 	const char *name = (char *) e + s->sh_offset;
 	const char *max = name + s->sh_size;
 
-	while (name < max)
+	while (name < max && *name) /* Segment may contain trailing 0.  */
 	  {
 	    grub_dl_t m;
 	    grub_dl_dep_t dep;

Re: [PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE)

[Robert Millan]

On Sat, Nov 10, 2007 at 08:03:57PM +0100, Christian Franke wrote:
> 
> This patch fixes this by ignoring the difference. It actually works to 
> load modules compiled on Linux by a kernel compiled on Cygwin (with 
> underscores) and vice versa.
>
> [...]
> +
> +  /* Ignore leading underscore used for C symbols.
> +     Done at runtime to allow loading modules compiled on other OS.  */
> +  if (*name == '_')
> +    name++;

Is that something we really want?  We never made any efforts at maintaining
ABI between kernel and modules, or in allowing this kind of combinations.  If
we go this path, it can mean more work later (e.g. support for building kernel
and modules with different versions of GCC, etc).

Could you give an example of a situation in which this would be useful?

-- 
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] Handle C symbols with leading underscore (HAVE_ASM_USCORE)

[Christian Franke]

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

Robert Millan wrote:
> On Sat, Nov 10, 2007 at 08:03:57PM +0100, Christian Franke wrote:
>   
>> This patch fixes this by ignoring the difference. It actually works to 
>> load modules compiled on Linux by a kernel compiled on Cygwin (with 
>> underscores) and vice versa.
>>
>> [...]
>> +
>> +  /* Ignore leading underscore used for C symbols.
>> +     Done at runtime to allow loading modules compiled on other OS.  */
>> +  if (*name == '_')
>> +    name++;
>>     
>
> Is that something we really want?  We never made any efforts at maintaining
> ABI between kernel and modules, or in allowing this kind of combinations.  If
> we go this path, it can mean more work later (e.g. support for building kernel
> and modules with different versions of GCC, etc).
>
>   

Agree.

Should the kernel probably check whether a module is build for the same 
target? This would avoid interesting behaviour if a foreign module is 
accidentally load for some reason. It could e.g. be done by checking a 
symbol derived from $target.


> Could you give an example of a situation in which this would be useful?
>
>   

There probably is none, therefore the (older) patch for the other 
variant is attached :-)

Christian

2007-11-11  Christian Franke  <franke@computer.org>

	* genkernsyms.sh.in: Handle HAVE_ASM_USCORE case.

	* genmk.rb: Handle HAVE_ASM_USCORE case in strip command.
	Add EXEEXT to CLEANFILES.

	* gensymlist.sh.in: Handle HAVE_ASM_USCORE case.

	* kern/dl.c (grub_dl_resolve_symbols): Add check for grub_mod_init
	and grub_mod_fini for symbols without a type. Handle HAVE_ASM_USCORE
	case for these symbols.
	(grub_dl_resolve_dependencies): Add check for trailing nullbytes
	in symbol table. This fixes an infinite loop if table is zero filled.



[-- Attachment #2: grub2-asm_uscore-2.patch --]
[-- Type: text/x-patch, Size: 5191 bytes --]

diff -rup grub2.orig/genkernsyms.sh.in grub2/genkernsyms.sh.in
--- grub2.orig/genkernsyms.sh.in	2006-07-12 22:42:52.000000000 +0200
+++ grub2/genkernsyms.sh.in	2007-10-06 12:59:29.000000000 +0200
@@ -16,9 +16,12 @@
 srcdir=@srcdir@
 CC="@CC@"
 
+u=
+grep "^#define HAVE_ASM_USCORE" config.h >/dev/null 2>&1 && u="_"
+
 $CC -DGRUB_SYMBOL_GENERATOR=1 -E -I. -Iinclude -I$srcdir/include $* \
   | grep -v '^#' \
   | sed -n \
-        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/\1 kernel/;p;}' \
-        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/\1 kernel/;p;}' \
+        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
+        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
   | sort -u
diff -rup grub2.orig/genmk.rb grub2/genmk.rb
--- grub2.orig/genmk.rb	2007-10-20 22:49:38.968750000 +0200
+++ grub2/genmk.rb	2007-11-10 19:37:17.562500000 +0100
@@ -115,7 +115,7 @@ UNDSYMFILES += #{undsym}
 #{@name}: #{pre_obj} #{mod_obj}
 	-rm -f $@
 	$(TARGET_CC) $(#{prefix}_LDFLAGS) $(TARGET_LDFLAGS) -Wl,-r,-d -o $@ $^
-	$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -R .note -R .comment $@
+	$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -K _grub_mod_init -K _grub_mod_fini -R .note -R .comment $@
 
 #{pre_obj}: $(#{prefix}_DEPENDENCIES) #{objs_str}
 	-rm -f $@
@@ -187,7 +187,7 @@ class Utility
     deps = objs.collect {|obj| obj.suffix('d')}
     deps_str = deps.join(' ');
 
-    "CLEANFILES += #{@name} #{objs_str}
+    "CLEANFILES += #{@name}$(EXEEXT) #{objs_str}
 MOSTLYCLEANFILES += #{deps_str}
 
 #{@name}: $(#{prefix}_DEPENDENCIES) #{objs_str}
diff -rup grub2.orig/gensymlist.sh.in grub2/gensymlist.sh.in
--- grub2.orig/gensymlist.sh.in	2007-07-22 01:32:18.000000000 +0200
+++ grub2/gensymlist.sh.in	2007-10-07 17:47:37.000000000 +0200
@@ -16,6 +16,8 @@
 srcdir=@srcdir@
 CC=@CC@
 
+u=
+grep "^#define HAVE_ASM_USCORE" config.h >/dev/null 2>&1 && u="_"
 
 cat <<EOF
 /* This file is automatically generated by gensymlist.sh. DO NOT EDIT! */
@@ -60,8 +62,8 @@ EOF
 $CC -DGRUB_SYMBOL_GENERATOR=1 -E -I. -Iinclude -I$srcdir/include $* \
   | grep -v '^#' \
   | sed -n \
-        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/      {"\1", \1},/;p;}' \
-        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/      {"\1", \&\1},/;p;}' \
+        -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/      {"'"$u"'\1", \1},/;p;}' \
+        -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/      {"'"$u"'\1", \&\1},/;p;}' \
   | sort -u
 
 cat <<EOF
diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c
--- grub2.orig/kern/dl.c	2007-07-22 01:32:26.000000000 +0200
+++ grub2/kern/dl.c	2007-11-11 18:01:50.578125000 +0100
@@ -53,6 +53,12 @@ typedef Elf64_Sym Elf_Sym;
 
 #endif
 
+#ifdef HAVE_ASM_USCORE
+# define SYM_USCORE "_"
+#else
+# define SYM_USCORE ""
+#endif
+
 \f
 
 struct grub_dl_list
@@ -347,6 +353,7 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
       unsigned char type = ELF_ST_TYPE (sym->st_info);
       unsigned char bind = ELF_ST_BIND (sym->st_info);
       const char *name = str + sym->st_name;
+      int check_mod_func = 0;
       
       switch (type)
 	{
@@ -359,6 +366,12 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 		return grub_error (GRUB_ERR_BAD_MODULE,
 				   "the symbol `%s' not found", name);
 	    }
+	  else if (sym->st_name != 0 && bind == STB_LOCAL)
+	    { /* Static functions have no type if initial format was not ELF.  */
+	      sym->st_value += (Elf_Addr) grub_dl_get_section_addr (mod,
+								    sym->st_shndx);
+	      check_mod_func = 1;
+	    }
 	  else
 	    sym->st_value = 0;
 	  break;
@@ -374,14 +387,11 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 	case STT_FUNC:
 	  sym->st_value += (Elf_Addr) grub_dl_get_section_addr (mod,
 								sym->st_shndx);
-	  if (bind != STB_LOCAL)
+	  if (bind == STB_LOCAL)
+	    check_mod_func = 1;
+	  else	
 	    if (grub_dl_register_symbol (name, (void *) sym->st_value, mod))
 	      return grub_errno;
-	  
-	  if (grub_strcmp (name, "grub_mod_init") == 0)
-	    mod->init = (void (*) (grub_dl_t)) sym->st_value;
-	  else if (grub_strcmp (name, "grub_mod_fini") == 0)
-	    mod->fini = (void (*) (void)) sym->st_value;
 	  break;
 
 	case STT_SECTION:
@@ -397,6 +407,13 @@ grub_dl_resolve_symbols (grub_dl_t mod, 
 	  return grub_error (GRUB_ERR_BAD_MODULE,
 			     "unknown symbol type `%d'", (int) type);
 	}
+      if (check_mod_func)
+        {
+	  if (grub_strcmp (name, SYM_USCORE "grub_mod_init") == 0)
+	    mod->init = (void (*) (grub_dl_t)) sym->st_value;
+	  else if (grub_strcmp (name, SYM_USCORE "grub_mod_fini") == 0)
+	    mod->fini = (void (*) (void)) sym->st_value;
+        }
     }
 
   return GRUB_ERR_NONE;
@@ -454,7 +471,7 @@ grub_dl_resolve_dependencies (grub_dl_t 
 	const char *name = (char *) e + s->sh_offset;
 	const char *max = name + s->sh_size;
 
-	while (name < max)
+	while (name < max && *name) /* Segment may contain trailing 0.  */
 	  {
 	    grub_dl_t m;
 	    grub_dl_dep_t dep;

Re: [PATCH] Handle C symbols with leading underscore (HAVE_ASM_USCORE)

[Robert Millan]

On Sun, Nov 11, 2007 at 07:24:52PM +0100, Christian Franke wrote:
> 
> Should the kernel probably check whether a module is build for the same 
> target? This would avoid interesting behaviour if a foreign module is 
> accidentally load for some reason. It could e.g. be done by checking a 
> symbol derived from $target.

It's important to keep the kernel small.  IMHO checks against improper use
themselves don't justify increasing its size (although what you want might
fit well in grub-mkimage, at least for modules that are put in core.img).

OTOH, there's an ongoing discussion on how to embed a filesystem image as
part of core.img, which (maybe, I'm not sure ;-)) could qualify as "foreign
module" and somehow collide with this.  Thread subject is:
"Re: grub-setup: error: Non-sector-aligned data is found in the core file"

> diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c
> --- grub2.orig/kern/dl.c	2007-07-22 01:32:26.000000000 +0200
> +++ grub2/kern/dl.c	2007-11-11 18:01:50.578125000 +0100
> @@ -53,6 +53,12 @@ typedef Elf64_Sym Elf_Sym;
>  
>  #endif
>  
> +#ifdef HAVE_ASM_USCORE
> +# define SYM_USCORE "_"
> +#else
> +# define SYM_USCORE ""
> +#endif

Should this be global?  We already have START_SYMBOL and END_SYMBOL.  Perhaps
this should be next to them?

-- 
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] Handle C symbols with leading underscore (HAVE_ASM_USCORE)

[Christian Franke]

Robert Millan wrote:
>
>> diff -rup grub2.orig/kern/dl.c grub2/kern/dl.c
>> --- grub2.orig/kern/dl.c	2007-07-22 01:32:26.000000000 +0200
>> +++ grub2/kern/dl.c	2007-11-11 18:01:50.578125000 +0100
>> @@ -53,6 +53,12 @@ typedef Elf64_Sym Elf_Sym;
>>  
>>  #endif
>>  
>> +#ifdef HAVE_ASM_USCORE
>> +# define SYM_USCORE "_"
>> +#else
>> +# define SYM_USCORE ""
>> +#endif
>>     
>
> Should this be global?  We already have START_SYMBOL and END_SYMBOL.  Perhaps
> this should be next to them?
>
>   

START_SYMBOL and END_SYMBOL are in config.h. SYM_USCORE is derived from 
a config.h symbol and therefore cannot be placed there without changing 
configure.ac.

The symbol is (and likely will be) only used once, so it IMO makes no 
sense to declare it non-locally.

Christian