* [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file
@ 2012-12-04 10:18 Michal Marek
2012-12-04 10:18 ` [PATCH 2/3] MODSIGN: Avoid using .incbin in C source Michal Marek
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Michal Marek @ 2012-12-04 10:18 UTC (permalink / raw)
To: dhowells; +Cc: linux-kernel, rusty
Signed-off-by: Michal Marek <mmarek@suse.cz>
---
scripts/sign-file | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/sign-file b/scripts/sign-file
index 87ca59d..974a20b 100755
--- a/scripts/sign-file
+++ b/scripts/sign-file
@@ -156,12 +156,12 @@ sub asn1_extract($$@)
if ($l == 0x1) {
$len = unpack("C", substr(${$cursor->[2]}, $cursor->[0], 1));
- } elsif ($l = 0x2) {
+ } elsif ($l == 0x2) {
$len = unpack("n", substr(${$cursor->[2]}, $cursor->[0], 2));
- } elsif ($l = 0x3) {
+ } elsif ($l == 0x3) {
$len = unpack("C", substr(${$cursor->[2]}, $cursor->[0], 1)) << 16;
$len = unpack("n", substr(${$cursor->[2]}, $cursor->[0] + 1, 2));
- } elsif ($l = 0x4) {
+ } elsif ($l == 0x4) {
$len = unpack("N", substr(${$cursor->[2]}, $cursor->[0], 4));
} else {
die $x509, ": ", $cursor->[0], ": ASN.1 element too long (", $l, ")\n";
--
1.7.10.4
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-04 10:18 [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Michal Marek @ 2012-12-04 10:18 ` Michal Marek 2012-12-04 10:18 ` [PATCH 3/3] MODSIGN: Drop ccache hack Michal Marek ` (4 subsequent siblings) 5 siblings, 0 replies; 20+ messages in thread From: Michal Marek @ 2012-12-04 10:18 UTC (permalink / raw) To: dhowells; +Cc: linux-kernel, rusty, Takashi Iwai From: Takashi Iwai <tiwai@suse.de> 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. Tested-by: Michal Marek <mmarek@suse.cz> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- kernel/Makefile | 4 ++-- kernel/modsign_certificate.S | 9 +++++++++ kernel/modsign_pubkey.c | 6 ------ 3 files changed, 11 insertions(+), 8 deletions(-) create mode 100644 kernel/modsign_certificate.S diff --git a/kernel/Makefile b/kernel/Makefile index 86e3285..0bd9d43 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -54,7 +54,7 @@ obj-$(CONFIG_DEBUG_SPINLOCK) += spinlock.o obj-$(CONFIG_PROVE_LOCKING) += spinlock.o obj-$(CONFIG_UID16) += uid16.o obj-$(CONFIG_MODULES) += module.o -obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o +obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o modsign_certificate.o obj-$(CONFIG_KALLSYMS) += kallsyms.o obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o obj-$(CONFIG_KEXEC) += kexec.o @@ -139,7 +139,7 @@ ifeq ($(CONFIG_MODULE_SIG),y) extra_certificates: touch $@ -kernel/modsign_pubkey.o: signing_key.x509 extra_certificates +kernel/modsign_certificate.o: signing_key.x509 extra_certificates ############################################################################### # diff --git a/kernel/modsign_certificate.S b/kernel/modsign_certificate.S new file mode 100644 index 0000000..6bde029 --- /dev/null +++ b/kernel/modsign_certificate.S @@ -0,0 +1,9 @@ +#include <linux/linkage.h> + + .section ".init.data","aw" + +GLOBAL(modsign_certificate_list) + .incbin "signing_key.x509" + .incbin "extra_certificates" +END(modsign_certificate_list) +GLOBAL(modsign_certificate_list_end) diff --git a/kernel/modsign_pubkey.c b/kernel/modsign_pubkey.c index 4646eb2..045504f 100644 --- a/kernel/modsign_pubkey.c +++ b/kernel/modsign_pubkey.c @@ -20,12 +20,6 @@ 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" - "modsign_certificate_list:\n" - ".incbin \"signing_key.x509\"\n" - ".incbin \"extra_certificates\"\n" - "modsign_certificate_list_end:" - ); /* * We need to make sure ccache doesn't cache the .o file as it doesn't notice -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] MODSIGN: Drop ccache hack 2012-12-04 10:18 [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Michal Marek 2012-12-04 10:18 ` [PATCH 2/3] MODSIGN: Avoid using .incbin in C source Michal Marek @ 2012-12-04 10:18 ` Michal Marek 2012-12-04 18:15 ` [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file David Howells ` (3 subsequent siblings) 5 siblings, 0 replies; 20+ messages in thread From: Michal Marek @ 2012-12-04 10:18 UTC (permalink / raw) To: dhowells; +Cc: linux-kernel, rusty The __TIME__ macro is not needed anymore, because the pubkey is included in a separate .S file. Signed-off-by: Michal Marek <mmarek@suse.cz> --- kernel/modsign_pubkey.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/kernel/modsign_pubkey.c b/kernel/modsign_pubkey.c index 045504f..df27eca 100644 --- a/kernel/modsign_pubkey.c +++ b/kernel/modsign_pubkey.c @@ -22,12 +22,6 @@ extern __initdata const u8 modsign_certificate_list[]; extern __initdata const u8 modsign_certificate_list_end[]; /* - * We need to make sure ccache doesn't cache the .o file as it doesn't notice - * if modsign.pub changes. - */ -static __initdata const char annoy_ccache[] = __TIME__ "foo"; - -/* * Load the compiled-in keys */ static __init int module_verify_init(void) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file 2012-12-04 10:18 [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Michal Marek 2012-12-04 10:18 ` [PATCH 2/3] MODSIGN: Avoid using .incbin in C source Michal Marek 2012-12-04 10:18 ` [PATCH 3/3] MODSIGN: Drop ccache hack Michal Marek @ 2012-12-04 18:15 ` David Howells 2012-12-04 18:17 ` [PATCH 2/3] MODSIGN: Avoid using .incbin in C source David Howells ` (2 subsequent siblings) 5 siblings, 0 replies; 20+ messages in thread From: David Howells @ 2012-12-04 18:15 UTC (permalink / raw) To: Michal Marek; +Cc: dhowells, linux-kernel, rusty Michal Marek <mmarek@suse.cz> wrote: > Signed-off-by: Michal Marek <mmarek@suse.cz> If only perl gave you warnings for this as gcc does... Acked-by: David Howells <dhowells@redhat.com> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-04 10:18 [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Michal Marek ` (2 preceding siblings ...) 2012-12-04 18:15 ` [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file David Howells @ 2012-12-04 18:17 ` David Howells 2012-12-04 23:58 ` Rusty Russell 2012-12-05 7:46 ` Takashi Iwai 2012-12-04 18:29 ` [PATCH 3/3] MODSIGN: Drop ccache hack David Howells 2012-12-04 23:44 ` [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Rusty Russell 5 siblings, 2 replies; 20+ messages in thread From: David Howells @ 2012-12-04 18:17 UTC (permalink / raw) To: Michal Marek; +Cc: dhowells, linux-kernel, rusty, Takashi Iwai 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> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-04 18:17 ` [PATCH 2/3] MODSIGN: Avoid using .incbin in C source David Howells @ 2012-12-04 23:58 ` Rusty Russell 2012-12-05 7:39 ` Takashi Iwai ` (2 more replies) 2012-12-05 7:46 ` Takashi Iwai 1 sibling, 3 replies; 20+ messages in thread From: Rusty Russell @ 2012-12-04 23:58 UTC (permalink / raw) To: David Howells, Michal Marek Cc: dhowells, linux-kernel, Takashi Iwai, James Hogan 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. Plus, we now have a patch from James (CC:d) to prepend CONFIG_SYMBOL_PREFIX to these as requird. I think this will have to be done more carefully... Thanks, Rusty. From: Takashi Iwai <tiwai@suse.de> 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. Tested-by: Michal Marek <mmarek@suse.cz> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- kernel/Makefile | 4 ++-- kernel/modsign_certificate.S | 9 +++++++++ kernel/modsign_pubkey.c | 6 ------ 3 files changed, 11 insertions(+), 8 deletions(-) create mode 100644 kernel/modsign_certificate.S diff --git a/kernel/Makefile b/kernel/Makefile index 86e3285..0bd9d43 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -54,7 +54,7 @@ obj-$(CONFIG_DEBUG_SPINLOCK) += spinlock.o obj-$(CONFIG_PROVE_LOCKING) += spinlock.o obj-$(CONFIG_UID16) += uid16.o obj-$(CONFIG_MODULES) += module.o -obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o +obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o modsign_certificate.o obj-$(CONFIG_KALLSYMS) += kallsyms.o obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o obj-$(CONFIG_KEXEC) += kexec.o @@ -139,7 +139,7 @@ ifeq ($(CONFIG_MODULE_SIG),y) extra_certificates: touch $@ -kernel/modsign_pubkey.o: signing_key.x509 extra_certificates +kernel/modsign_certificate.o: signing_key.x509 extra_certificates ############################################################################### # diff --git a/kernel/modsign_certificate.S b/kernel/modsign_certificate.S new file mode 100644 index 0000000..6bde029 --- /dev/null +++ b/kernel/modsign_certificate.S @@ -0,0 +1,9 @@ +#include <linux/linkage.h> + + .section ".init.data","aw" + +GLOBAL(modsign_certificate_list) + .incbin "signing_key.x509" + .incbin "extra_certificates" +END(modsign_certificate_list) +GLOBAL(modsign_certificate_list_end) diff --git a/kernel/modsign_pubkey.c b/kernel/modsign_pubkey.c index 4646eb2..045504f 100644 --- a/kernel/modsign_pubkey.c +++ b/kernel/modsign_pubkey.c @@ -20,12 +20,6 @@ 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" - "modsign_certificate_list:\n" - ".incbin \"signing_key.x509\"\n" - ".incbin \"extra_certificates\"\n" - "modsign_certificate_list_end:" - ); /* * We need to make sure ccache doesn't cache the .o file as it doesn't notice -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-04 23:58 ` Rusty Russell @ 2012-12-05 7:39 ` Takashi Iwai 2012-12-05 9:50 ` Michal Marek 2012-12-05 10:30 ` David Howells 2012-12-05 12:35 ` David Howells 2 siblings, 1 reply; 20+ messages in thread From: Takashi Iwai @ 2012-12-05 7:39 UTC (permalink / raw) To: Rusty Russell; +Cc: David Howells, Michal Marek, linux-kernel, James Hogan 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. > > Plus, we now have a patch from James (CC:d) to prepend > CONFIG_SYMBOL_PREFIX to these as requird. Yes, I noticed both things yesterday. > > I think this will have to be done more carefully... How about the revised patch below? thanks, Takashi --- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH v2] MODSIGN: Avoid using .incbin in C source 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. Tested-by: Michal Marek <mmarek@suse.cz> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- kernel/Makefile | 4 ++-- kernel/modsign_certificate.S | 18 ++++++++++++++++++ kernel/modsign_pubkey.c | 6 ------ 3 files changed, 20 insertions(+), 8 deletions(-) create mode 100644 kernel/modsign_certificate.S diff --git a/kernel/Makefile b/kernel/Makefile index 86e3285..0bd9d43 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -54,7 +54,7 @@ obj-$(CONFIG_DEBUG_SPINLOCK) += spinlock.o obj-$(CONFIG_PROVE_LOCKING) += spinlock.o obj-$(CONFIG_UID16) += uid16.o obj-$(CONFIG_MODULES) += module.o -obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o +obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o modsign_certificate.o obj-$(CONFIG_KALLSYMS) += kallsyms.o obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o obj-$(CONFIG_KEXEC) += kexec.o @@ -139,7 +139,7 @@ ifeq ($(CONFIG_MODULE_SIG),y) extra_certificates: touch $@ -kernel/modsign_pubkey.o: signing_key.x509 extra_certificates +kernel/modsign_certificate.o: signing_key.x509 extra_certificates ############################################################################### # 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 + +#define GLOBAL(name) \ + .globl ASM_SYMBOL(name); \ + ASM_SYMBOL(name): + + .section ".init.data","aw" + +GLOBAL(modsign_certificate_list) + .incbin "signing_key.x509" + .incbin "extra_certificates" +GLOBAL(modsign_certificate_list_end) diff --git a/kernel/modsign_pubkey.c b/kernel/modsign_pubkey.c index 767e559..045504f 100644 --- a/kernel/modsign_pubkey.c +++ b/kernel/modsign_pubkey.c @@ -20,12 +20,6 @@ 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" - ".incbin \"signing_key.x509\"\n" - ".incbin \"extra_certificates\"\n" - SYMBOL_PREFIX "modsign_certificate_list_end:" - ); /* * We need to make sure ccache doesn't cache the .o file as it doesn't notice -- 1.8.0.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-05 7:39 ` Takashi Iwai @ 2012-12-05 9:50 ` Michal Marek 2012-12-05 10:06 ` Takashi Iwai ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Michal Marek @ 2012-12-05 9:50 UTC (permalink / raw) To: Takashi Iwai; +Cc: Rusty Russell, David Howells, linux-kernel, James Hogan 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-05 9:50 ` Michal Marek @ 2012-12-05 10:06 ` Takashi Iwai 2012-12-05 10:30 ` James Hogan 2012-12-07 4:40 ` Rusty Russell 2 siblings, 0 replies; 20+ messages in thread From: Takashi Iwai @ 2012-12-05 10:06 UTC (permalink / raw) To: Michal Marek; +Cc: Rusty Russell, David Howells, linux-kernel, James Hogan At Wed, 5 Dec 2012 10:50:57 +0100, Michal Marek wrote: > > 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 Urgh. Strange that I didn't notice the error when I tested with the patch (with artificial CONFIG_SYMBOL_PREFIX set in arch/x86/Kconfig)... > 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> Looks like a good cleanup. Reviewed-by: Takashi Iwai <tiwai@suse.de> Takashi > --- > 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 > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-05 9:50 ` Michal Marek 2012-12-05 10:06 ` Takashi Iwai @ 2012-12-05 10:30 ` James Hogan 2012-12-05 11:05 ` Michal Marek 2012-12-07 4:40 ` Rusty Russell 2 siblings, 1 reply; 20+ messages in thread From: James Hogan @ 2012-12-05 10:30 UTC (permalink / raw) To: Michal Marek; +Cc: Takashi Iwai, Rusty Russell, David Howells, linux-kernel On 05/12/12 09:50, Michal Marek wrote: >> 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. Hi Michal, Yeh, that looks good to me, and I like it just being automatically defined rather than having to include linux/kernel.h (potentially from low level architecture headers too). However I think it's unfortunate having to stringify from C as it's pretty much always required to be in string form when used from a C file, usually in an asm block. Any objection to defining SYMBOL_PREFIX with the quotes around it for _c_flags only? E.g. see below Cheers James > 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 > How about this instead (i.e. no changes to kernel/modsign_pubkey.c)? diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 0be6f11..a76967e 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -119,11 +119,11 @@ _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_sym_flags = -DSYMBOL_PREFIX=\"$(patsubst "%",%,$(CONFIG_SYMBOL_PREFIX))\" +_c_flags += $(_c_sym_flags) _a_flags += $(_sym_flags) -endif +_cpp_flags += $(_sym_flags) # If building the kernel in a separate objtree expand all occurrences ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-05 10:30 ` James Hogan @ 2012-12-05 11:05 ` Michal Marek 2012-12-05 11:16 ` James Hogan 0 siblings, 1 reply; 20+ messages in thread From: Michal Marek @ 2012-12-05 11:05 UTC (permalink / raw) To: James Hogan; +Cc: Takashi Iwai, Rusty Russell, David Howells, linux-kernel Dne 5.12.2012 11:30, James Hogan napsal(a): > However I think it's unfortunate having to stringify from C as it's > pretty much always required to be in string form when used from a C > file, usually in an asm block. Any objection to defining SYMBOL_PREFIX > with the quotes around it for _c_flags only? E.g. see below After Takashi's patch is applied, there will be no user of the SYMBOL_PREFIX in C source. But it would probably be more convenient for potential new users. > +_c_sym_flags = -DSYMBOL_PREFIX=\"$(patsubst "%",%,$(CONFIG_SYMBOL_PREFIX))\" Maybe simply -DDSYMBOL_PREFIX='$(or $(CONFIG_SYMBOL_PREFIX),"")' ? The single quotes are needed either way, to prevent the shell eating the double quotes. Michal ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-05 11:05 ` Michal Marek @ 2012-12-05 11:16 ` James Hogan 0 siblings, 0 replies; 20+ messages in thread From: James Hogan @ 2012-12-05 11:16 UTC (permalink / raw) To: Michal Marek; +Cc: Takashi Iwai, Rusty Russell, David Howells, linux-kernel On 05/12/12 11:05, Michal Marek wrote: > Dne 5.12.2012 11:30, James Hogan napsal(a): >> However I think it's unfortunate having to stringify from C as it's >> pretty much always required to be in string form when used from a C >> file, usually in an asm block. Any objection to defining SYMBOL_PREFIX >> with the quotes around it for _c_flags only? E.g. see below > > > After Takashi's patch is applied, there will be no user of the > SYMBOL_PREFIX in C source. But it would probably be more convenient for > potential new users. Yes indeed (so the C related bits could indeed be dropped for now). I'd likely change this patch to use it though: http://thread.gmane.org/gmane.linux.kernel.cross-arch/15586/focus=15593 > >> +_c_sym_flags = -DSYMBOL_PREFIX=\"$(patsubst "%",%,$(CONFIG_SYMBOL_PREFIX))\" > > Maybe simply > > -DDSYMBOL_PREFIX='$(or $(CONFIG_SYMBOL_PREFIX),"")' > > ? The single quotes are needed either way, to prevent the shell eating > the double quotes. Yep, works for me (makefiles aren't one of my strengths!) Cheers James ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-05 9:50 ` Michal Marek 2012-12-05 10:06 ` Takashi Iwai 2012-12-05 10:30 ` James Hogan @ 2012-12-07 4:40 ` Rusty Russell 2012-12-10 10:12 ` James Hogan 2 siblings, 1 reply; 20+ messages in thread From: Rusty Russell @ 2012-12-07 4:40 UTC (permalink / raw) To: Michal Marek, Takashi Iwai; +Cc: David Howells, linux-kernel, James Hogan Michal Marek <mmarek@suse.cz> writes: > 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 What a mess! AFAICT there's one place which a non-string SYMBOL_PREFIX: include/asm-generic/vmlinux.lds.h So we do need it, and the only sane way seems to be via the cmdline and the shell usefully stripping quotes as we do. So I just took Takashi's do-it-all-in-asm fix, and removed this re-definition (we have the same thing, BTW, called MODULE_SYMBOL_PREFIX). I added a comment about the cmdline SYMBOL_PREFIX, too. Thanks, Rusty. From: Takashi Iwai <tiwai@suse.de> Subject: MODSIGN: Avoid using .incbin in C source 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. [ This means we no longer need SYMBOL_PREFIX which is defined in kernel.h from cbdbf2abb7844548a7d7a6a2ae7af6b6fbcea401, so I removed it -- RR ] Tested-by: Michal Marek <mmarek@suse.cz> Signed-off-by: Takashi Iwai <tiwai@suse.de> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 7d8dfc7..a123b13 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -701,13 +701,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } #define COMPACTION_BUILD 0 #endif -/* 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/Makefile b/kernel/Makefile index 86e3285..0bd9d43 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -54,7 +54,7 @@ obj-$(CONFIG_DEBUG_SPINLOCK) += spinlock.o obj-$(CONFIG_PROVE_LOCKING) += spinlock.o obj-$(CONFIG_UID16) += uid16.o obj-$(CONFIG_MODULES) += module.o -obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o +obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o modsign_certificate.o obj-$(CONFIG_KALLSYMS) += kallsyms.o obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o obj-$(CONFIG_KEXEC) += kexec.o @@ -139,7 +139,7 @@ ifeq ($(CONFIG_MODULE_SIG),y) extra_certificates: touch $@ -kernel/modsign_pubkey.o: signing_key.x509 extra_certificates +kernel/modsign_certificate.o: signing_key.x509 extra_certificates ############################################################################### # diff --git a/kernel/modsign_certificate.S b/kernel/modsign_certificate.S new file mode 100644 index 0000000..246b4c6 --- /dev/null +++ b/kernel/modsign_certificate.S @@ -0,0 +1,19 @@ +/* SYMBOL_PREFIX defined on commandline from CONFIG_SYMBOL_PREFIX */ +#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 + +#define GLOBAL(name) \ + .globl ASM_SYMBOL(name); \ + ASM_SYMBOL(name): + + .section ".init.data","aw" + +GLOBAL(modsign_certificate_list) + .incbin "signing_key.x509" + .incbin "extra_certificates" +GLOBAL(modsign_certificate_list_end) diff --git a/kernel/modsign_pubkey.c b/kernel/modsign_pubkey.c index 767e559..045504f 100644 --- a/kernel/modsign_pubkey.c +++ b/kernel/modsign_pubkey.c @@ -20,12 +20,6 @@ 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" - ".incbin \"signing_key.x509\"\n" - ".incbin \"extra_certificates\"\n" - SYMBOL_PREFIX "modsign_certificate_list_end:" - ); /* * We need to make sure ccache doesn't cache the .o file as it doesn't notice ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-07 4:40 ` Rusty Russell @ 2012-12-10 10:12 ` James Hogan 0 siblings, 0 replies; 20+ messages in thread From: James Hogan @ 2012-12-10 10:12 UTC (permalink / raw) To: Rusty Russell; +Cc: Michal Marek, Takashi Iwai, David Howells, linux-kernel On 07/12/12 04:40, Rusty Russell wrote: > Michal Marek <mmarek@suse.cz> writes: >> 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 > > What a mess! AFAICT there's one place which a non-string SYMBOL_PREFIX: > include/asm-generic/vmlinux.lds.h > > So we do need it, and the only sane way seems to be via the cmdline and > the shell usefully stripping quotes as we do. > > So I just took Takashi's do-it-all-in-asm fix, and removed this > re-definition (we have the same thing, BTW, called > MODULE_SYMBOL_PREFIX). > > I added a comment about the cmdline SYMBOL_PREFIX, too. > > Thanks, > Rusty. > > From: Takashi Iwai <tiwai@suse.de> > Subject: MODSIGN: Avoid using .incbin in C source > > 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. > > [ This means we no longer need SYMBOL_PREFIX which is defined in kernel.h > from cbdbf2abb7844548a7d7a6a2ae7af6b6fbcea401, so I removed it -- RR ] > > Tested-by: Michal Marek <mmarek@suse.cz> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Looks good to me. Acked-by: James Hogan <james.hogan@imgtec.com> Thanks James > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index 7d8dfc7..a123b13 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -701,13 +701,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } > #define COMPACTION_BUILD 0 > #endif > > -/* 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/Makefile b/kernel/Makefile > index 86e3285..0bd9d43 100644 > --- a/kernel/Makefile > +++ b/kernel/Makefile > @@ -54,7 +54,7 @@ obj-$(CONFIG_DEBUG_SPINLOCK) += spinlock.o > obj-$(CONFIG_PROVE_LOCKING) += spinlock.o > obj-$(CONFIG_UID16) += uid16.o > obj-$(CONFIG_MODULES) += module.o > -obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o > +obj-$(CONFIG_MODULE_SIG) += module_signing.o modsign_pubkey.o modsign_certificate.o > obj-$(CONFIG_KALLSYMS) += kallsyms.o > obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o > obj-$(CONFIG_KEXEC) += kexec.o > @@ -139,7 +139,7 @@ ifeq ($(CONFIG_MODULE_SIG),y) > extra_certificates: > touch $@ > > -kernel/modsign_pubkey.o: signing_key.x509 extra_certificates > +kernel/modsign_certificate.o: signing_key.x509 extra_certificates > > ############################################################################### > # > diff --git a/kernel/modsign_certificate.S b/kernel/modsign_certificate.S > new file mode 100644 > index 0000000..246b4c6 > --- /dev/null > +++ b/kernel/modsign_certificate.S > @@ -0,0 +1,19 @@ > +/* SYMBOL_PREFIX defined on commandline from CONFIG_SYMBOL_PREFIX */ > +#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 > + > +#define GLOBAL(name) \ > + .globl ASM_SYMBOL(name); \ > + ASM_SYMBOL(name): > + > + .section ".init.data","aw" > + > +GLOBAL(modsign_certificate_list) > + .incbin "signing_key.x509" > + .incbin "extra_certificates" > +GLOBAL(modsign_certificate_list_end) > diff --git a/kernel/modsign_pubkey.c b/kernel/modsign_pubkey.c > index 767e559..045504f 100644 > --- a/kernel/modsign_pubkey.c > +++ b/kernel/modsign_pubkey.c > @@ -20,12 +20,6 @@ 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" > - ".incbin \"signing_key.x509\"\n" > - ".incbin \"extra_certificates\"\n" > - SYMBOL_PREFIX "modsign_certificate_list_end:" > - ); > > /* > * We need to make sure ccache doesn't cache the .o file as it doesn't notice > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-04 23:58 ` Rusty Russell 2012-12-05 7:39 ` Takashi Iwai @ 2012-12-05 10:30 ` David Howells 2012-12-05 10:54 ` Michal Marek 2012-12-05 12:35 ` David Howells 2 siblings, 1 reply; 20+ messages in thread From: David Howells @ 2012-12-05 10:30 UTC (permalink / raw) To: Takashi Iwai Cc: dhowells, Rusty Russell, Michal Marek, linux-kernel, James Hogan Takashi Iwai <tiwai@suse.de> wrote: > +#ifndef SYMBOL_PREFIX This comes via the command line? David ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-05 10:30 ` David Howells @ 2012-12-05 10:54 ` Michal Marek 0 siblings, 0 replies; 20+ messages in thread From: Michal Marek @ 2012-12-05 10:54 UTC (permalink / raw) To: David Howells; +Cc: Takashi Iwai, Rusty Russell, linux-kernel, James Hogan Dne 5.12.2012 11:30, David Howells napsal(a): > Takashi Iwai <tiwai@suse.de> wrote: > >> +#ifndef SYMBOL_PREFIX > > This comes via the command line? Yes, see scripts/Makefile.lib: ifdef CONFIG_SYMBOL_PREFIX _sym_flags = -DSYMBOL_PREFIX=$(patsubst "%",%,$(CONFIG_SYMBOL_PREFIX)) _cpp_flags += $(_sym_flags) _a_flags += $(_sym_flags) endif Michal ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-04 23:58 ` Rusty Russell 2012-12-05 7:39 ` Takashi Iwai 2012-12-05 10:30 ` David Howells @ 2012-12-05 12:35 ` David Howells 2 siblings, 0 replies; 20+ messages in thread From: David Howells @ 2012-12-05 12:35 UTC (permalink / raw) To: Takashi Iwai Cc: dhowells, Rusty Russell, Michal Marek, linux-kernel, James Hogan Takashi Iwai <tiwai@suse.de> wrote: > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH v2] MODSIGN: Avoid using .incbin in C source > > 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. > > Tested-by: Michal Marek <mmarek@suse.cz> > Signed-off-by: Takashi Iwai <tiwai@suse.de> Acked-by: David Howells <dhowells@redhat.com> > +#define GLOBAL(name) \ > + .globl ASM_SYMBOL(name); \ > + ASM_SYMBOL(name): This should perhaps be moved into a global header. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source 2012-12-04 18:17 ` [PATCH 2/3] MODSIGN: Avoid using .incbin in C source David Howells 2012-12-04 23:58 ` Rusty Russell @ 2012-12-05 7:46 ` Takashi Iwai 1 sibling, 0 replies; 20+ messages in thread From: Takashi Iwai @ 2012-12-05 7:46 UTC (permalink / raw) To: David Howells; +Cc: Michal Marek, linux-kernel, rusty, Takashi Iwai At Tue, 04 Dec 2012 18:17:28 +0000, David Howells wrote: > > 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 Well, we hit actually the issue... Please check the revised patch again. thanks, Takashi > - so: > > Acked-by: David Howells <dhowells@redhat.com> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] MODSIGN: Drop ccache hack 2012-12-04 10:18 [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Michal Marek ` (3 preceding siblings ...) 2012-12-04 18:17 ` [PATCH 2/3] MODSIGN: Avoid using .incbin in C source David Howells @ 2012-12-04 18:29 ` David Howells 2012-12-04 23:44 ` [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Rusty Russell 5 siblings, 0 replies; 20+ messages in thread From: David Howells @ 2012-12-04 18:29 UTC (permalink / raw) To: Michal Marek; +Cc: dhowells, linux-kernel, rusty Michal Marek <mmarek@suse.cz> wrote: > The __TIME__ macro is not needed anymore, because the pubkey is included > in a separate .S file. Acked-by: David Howells <dhowells@redhat.com> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file 2012-12-04 10:18 [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Michal Marek ` (4 preceding siblings ...) 2012-12-04 18:29 ` [PATCH 3/3] MODSIGN: Drop ccache hack David Howells @ 2012-12-04 23:44 ` Rusty Russell 5 siblings, 0 replies; 20+ messages in thread From: Rusty Russell @ 2012-12-04 23:44 UTC (permalink / raw) To: Michal Marek, dhowells; +Cc: linux-kernel Michal Marek <mmarek@suse.cz> writes: > Signed-off-by: Michal Marek <mmarek@suse.cz> > --- > scripts/sign-file | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/sign-file b/scripts/sign-file > index 87ca59d..974a20b 100755 > --- a/scripts/sign-file > +++ b/scripts/sign-file > @@ -156,12 +156,12 @@ sub asn1_extract($$@) > > if ($l == 0x1) { > $len = unpack("C", substr(${$cursor->[2]}, $cursor->[0], 1)); > - } elsif ($l = 0x2) { > + } elsif ($l == 0x2) { > $len = unpack("n", substr(${$cursor->[2]}, $cursor->[0], 2)); > - } elsif ($l = 0x3) { > + } elsif ($l == 0x3) { > $len = unpack("C", substr(${$cursor->[2]}, $cursor->[0], 1)) << 16; > $len = unpack("n", substr(${$cursor->[2]}, $cursor->[0] + 1, 2)); > - } elsif ($l = 0x4) { > + } elsif ($l == 0x4) { > $len = unpack("N", substr(${$cursor->[2]}, $cursor->[0], 4)); > } else { > die $x509, ": ", $cursor->[0], ": ASN.1 element too long (", $l, ")\n"; This was already applied to Linus' tree as commit 916492b1e1a186260951831c53a53d8a448dc026 Author: Chun-Yi Lee <jlee@suse.com> Date: Wed Nov 21 11:26:09 2012 +0000 Thanks, Rusty. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2012-12-10 10:13 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-12-04 10:18 [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Michal Marek 2012-12-04 10:18 ` [PATCH 2/3] MODSIGN: Avoid using .incbin in C source Michal Marek 2012-12-04 10:18 ` [PATCH 3/3] MODSIGN: Drop ccache hack Michal Marek 2012-12-04 18:15 ` [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file David Howells 2012-12-04 18:17 ` [PATCH 2/3] MODSIGN: Avoid using .incbin in C source David Howells 2012-12-04 23:58 ` Rusty Russell 2012-12-05 7:39 ` Takashi Iwai 2012-12-05 9:50 ` Michal Marek 2012-12-05 10:06 ` Takashi Iwai 2012-12-05 10:30 ` James Hogan 2012-12-05 11:05 ` Michal Marek 2012-12-05 11:16 ` James Hogan 2012-12-07 4:40 ` Rusty Russell 2012-12-10 10:12 ` James Hogan 2012-12-05 10:30 ` David Howells 2012-12-05 10:54 ` Michal Marek 2012-12-05 12:35 ` David Howells 2012-12-05 7:46 ` Takashi Iwai 2012-12-04 18:29 ` [PATCH 3/3] MODSIGN: Drop ccache hack David Howells 2012-12-04 23:44 ` [PATCH 1/3] MODSIGN: Fix comparison erros in scripts/sign-file Rusty Russell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).