Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source

[Michal Marek <mmarek@suse.cz>]

On Wed, Dec 05, 2012 at 08:39:11AM +0100, Takashi Iwai wrote:
> At Wed, 05 Dec 2012 10:28:48 +1030,
> Rusty Russell wrote:
> > 
> > David Howells <dhowells@redhat.com> writes:
> > 
> > > Michal Marek <mmarek@suse.cz> wrote:
> > >
> > >> Using the asm .incbin statement in C sources breaks any gcc wrapper which
> > >> assumes that preprocessed C source is self-contained. Use a separate .S
> > >> file to include the siging key and certificate.
> > >
> > > I was trying to avoid that as .S files generally don't crop up in generic
> > > code and the format of the assembly varies with the arch.  However, you don't
> > > seem to have anything that should cause a problem - so:
> > >
> > > Acked-by: David Howells <dhowells@redhat.com>
> > 
> > GLOBAL() is defined in x86 only, AFAICT.

Sorry for that, the Tested-by actually meant Tested-on-x86_64-by :/


> How about the revised patch below?
[...]
> diff --git a/kernel/modsign_certificate.S b/kernel/modsign_certificate.S
> new file mode 100644
> index 0000000..695d4e3
> --- /dev/null
> +++ b/kernel/modsign_certificate.S
> @@ -0,0 +1,18 @@
> +#ifndef SYMBOL_PREFIX
> +#define ASM_SYMBOL(sym) sym
> +#else
> +#define PASTE2(x,y) x##y
> +#define PASTE(x,y) PASTE2(x,y)
> +#define ASM_SYMBOL(sym) PASTE(SYMBOL_PREFIX, sym)
> +#endif


It looks good, but looking at your patch, I just noticed that we have two
versions of the SYMBOL_PREFIX macro in the kernel now:

scripts/Makefile.lib has had since some time

ifdef CONFIG_SYMBOL_PREFIX
_sym_flags = -DSYMBOL_PREFIX=$(patsubst "%",%,$(CONFIG_SYMBOL_PREFIX))
_cpp_flags += $(_sym_flags)
_a_flags += $(_sym_flags)
endif

while include/linux/kernel.h now has

/* This helps us to avoid #ifdef CONFIG_SYMBOL_PREFIX */
#ifdef CONFIG_SYMBOL_PREFIX
#define SYMBOL_PREFIX CONFIG_SYMBOL_PREFIX
#else
#define SYMBOL_PREFIX ""
#endif

So depending on whether you include <linux/kernel.h> or not,
SYMBOL_PREFIX expands to either a string or a token. James, Rusty, how
about the patch below? If Takashi's patch is applied first, then
obviously the modsign_pubkey.c part should be dropped and the ASM_SYMBOL
definition in modsign_certificate.S can be simplified instead.

Michal

>From df545c46fb89c085387f4d98c7a4fb06a7f678a9 Mon Sep 17 00:00:00 2001
From: Michal Marek <mmarek@suse.cz>
Date: Wed, 5 Dec 2012 10:03:15 +0100
Subject: [PATCH] linux/kernel.h: Avoid conflicting SYMBOL_PREFIX definition

scripts/Makefile.lib already defines SYMBOL_PREFIX to the token _ if
CONFIG_SYMBOL_PREFIX is defined. Drop the string definition in
linux/kernel.h and change scripts/Makefile.lib to define SYMBOL_PREFIX
on all architectures.

Signed-off-by: Michal Marek <mmarek@suse.cz>
---
 include/asm-generic/vmlinux.lds.h |    4 ----
 include/linux/kernel.h            |    7 -------
 kernel/modsign_pubkey.c           |    5 +++--
 scripts/Makefile.lib              |    5 ++---
 4 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index d1ea7ce..7756a0c 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -52,13 +52,9 @@
 #define LOAD_OFFSET 0
 #endif
 
-#ifndef SYMBOL_PREFIX
-#define VMLINUX_SYMBOL(sym) sym
-#else
 #define PASTE2(x,y) x##y
 #define PASTE(x,y) PASTE2(x,y)
 #define VMLINUX_SYMBOL(sym) PASTE(SYMBOL_PREFIX, sym)
-#endif
 
 /* Align . to a 8 byte boundary equals to maximum function alignment. */
 #define ALIGN_FUNCTION()  . = ALIGN(8)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d140e8f..9a6bccb 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -717,13 +717,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 /* Trap pasters of __FUNCTION__ at compile-time */
 #define __FUNCTION__ (__func__)
 
-/* This helps us to avoid #ifdef CONFIG_SYMBOL_PREFIX */
-#ifdef CONFIG_SYMBOL_PREFIX
-#define SYMBOL_PREFIX CONFIG_SYMBOL_PREFIX
-#else
-#define SYMBOL_PREFIX ""
-#endif
-
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
diff --git a/kernel/modsign_pubkey.c b/kernel/modsign_pubkey.c
index 767e559..93ce84b 100644
--- a/kernel/modsign_pubkey.c
+++ b/kernel/modsign_pubkey.c
@@ -9,6 +9,7 @@
  * 2 of the Licence, or (at your option) any later version.
  */
 
+#include <linux/stringify.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/cred.h>
@@ -21,10 +22,10 @@ struct key *modsign_keyring;
 extern __initdata const u8 modsign_certificate_list[];
 extern __initdata const u8 modsign_certificate_list_end[];
 asm(".section .init.data,\"aw\"\n"
-    SYMBOL_PREFIX "modsign_certificate_list:\n"
+    __stringify(SYMBOL_PREFIX) "modsign_certificate_list:\n"
     ".incbin \"signing_key.x509\"\n"
     ".incbin \"extra_certificates\"\n"
-    SYMBOL_PREFIX "modsign_certificate_list_end:"
+    __stringify(SYMBOL_PREFIX) "modsign_certificate_list_end:"
     );
 
 /*
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index bdf42fd..1138e77 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -119,11 +119,10 @@ _c_flags += $(if $(patsubst n%,, \
 		$(CFLAGS_GCOV))
 endif
 
-ifdef CONFIG_SYMBOL_PREFIX
 _sym_flags = -DSYMBOL_PREFIX=$(patsubst "%",%,$(CONFIG_SYMBOL_PREFIX))
-_cpp_flags += $(_sym_flags)
+_c_flags += $(_sym_flags)
 _a_flags += $(_sym_flags)
-endif
+_cpp_flags += $(_sym_flags)
 
 
 # If building the kernel in a separate objtree expand all occurrences
-- 
1.7.10.4