| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <s5hhao0vmmb.wl%tiwai@suse.de>
Date: Wed, 05 Dec 2012 11:06:52 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Michal Marek <mmarek@...e.cz>
Cc: Rusty Russell <rusty@...abs.org>,
David Howells <dhowells@...hat.com>,
linux-kernel@...r.kernel.org, James Hogan <james.hogan@...tec.com>
Subject: Re: [PATCH 2/3] MODSIGN: Avoid using .incbin in C source
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@...hat.com> writes:
> > >
> > > > Michal Marek <mmarek@...e.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@...hat.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@...e.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@...e.cz>
Looks like a good cleanup.
Reviewed-by: Takashi Iwai <tiwai@...e.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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists