[PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[Greg Kroah-Hartman]

This reverts commit 6cfb521ac0d5b97470883ff9b7facae264b7ab12.

Turns out distros do not want to make retpoline as part of their "ABI",
so this patch should not have been merged.  Sorry Andi, this was my
fault, I suggested it when your original patch was the "correct" way of
doing this instead.

Reported-by: Jiri Kosina <jikos@kernel.org>
Fixes: 6cfb521ac0d5 ("module: Add retpoline tag to VERMAGIC")
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: rusty@rustcorp.com.au
Cc: arjan.van.de.ven@intel.com
Cc: jeyu@kernel.org
Cc: torvalds@linux-foundation.org
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

Linus, if there are no objections, can you apply this revert to your
tree now so this doesn't get into 4.15?  It's already in the stable
releases but Jiri pointed out he instantly reverted it for SuSE's
kernels as it does not do what they want it to do.  They want to just
taint the kernel, not prevent modules from being loaded entirely.

 include/linux/vermagic.h | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h
index 853291714ae0..bae807eb2933 100644
--- a/include/linux/vermagic.h
+++ b/include/linux/vermagic.h
@@ -31,17 +31,11 @@
 #else
 #define MODULE_RANDSTRUCT_PLUGIN
 #endif
-#ifdef RETPOLINE
-#define MODULE_VERMAGIC_RETPOLINE "retpoline "
-#else
-#define MODULE_VERMAGIC_RETPOLINE ""
-#endif
 
 #define VERMAGIC_STRING 						\
 	UTS_RELEASE " "							\
 	MODULE_VERMAGIC_SMP MODULE_VERMAGIC_PREEMPT 			\
 	MODULE_VERMAGIC_MODULE_UNLOAD MODULE_VERMAGIC_MODVERSIONS	\
 	MODULE_ARCH_VERMAGIC						\
-	MODULE_RANDSTRUCT_PLUGIN					\
-	MODULE_VERMAGIC_RETPOLINE
+	MODULE_RANDSTRUCT_PLUGIN
 
-- 
2.16.1

Re: [PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[Andi Kleen]

On Wed, Jan 24, 2018 at 03:28:17PM +0100, Greg Kroah-Hartman wrote:
> This reverts commit 6cfb521ac0d5b97470883ff9b7facae264b7ab12.
> 
> Turns out distros do not want to make retpoline as part of their "ABI",
> so this patch should not have been merged.  Sorry Andi, this was my
> fault, I suggested it when your original patch was the "correct" way of
> doing this instead.

It's ok for me to revert, but if it's reverted should consider
the warning only patch version instead.

https://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-misc.git/commit/?h=spec/retpoline-module-2&id=b40b7c1dbffaa636906b774ef5519882060e1aa0

I had posted that earlier, but reviewers in the end preferred
the simple modversions variant.

I think we should have something at least for 4.15 to catch mistakes.

-Andi

Re: [PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[Linus Torvalds]

On Wed, Jan 24, 2018 at 6:28 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Linus, if there are no objections, can you apply this revert to your
> tree now so this doesn't get into 4.15?

Applied.

            Linus

Re: [PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[Andi Kleen]

On Wed, Jan 24, 2018 at 09:00:48AM -0800, Linus Torvalds wrote:
> On Wed, Jan 24, 2018 at 6:28 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > Linus, if there are no objections, can you apply this revert to your
> > tree now so this doesn't get into 4.15?
> 
> Applied.

So can we get the warning replacement? It would be good to have some
kind of solution.

-Andi

----

retpoline/module: Warn for missing retpoline in module

There's a risk that a kernel that has full retpoline mitigations
becomes vulnerable when a module gets loaded that hasn't been
compiled with the right compiler or the right option.

We cannot fix it, but should at least warn the user when that
happens.

When the a module hasn't been compiled with a retpoline
aware compiler, print a warning and change the SPECTRE_V2
mitigation mode to show the system is vulnerable now.

For modules it is checked at compile time, however it cannot
check assembler or other non compiled objects used in the module link.

v2: Change warning message
v3: Port to latest tree
v4: Remove tainting
Cc: jeyu@kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 9c18da64daa9..ea707c91bd8c 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -970,4 +970,8 @@ bool xen_set_default_idle(void);
 
 void stop_this_cpu(void *dummy);
 void df_debug(struct pt_regs *regs, long error_code);
+
+void disable_retpoline(void);
+bool retpoline_enabled(void);
+
 #endif /* _ASM_X86_PROCESSOR_H */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index e4dc26185aa7..9064b20473a7 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -93,6 +93,18 @@ static const char *spectre_v2_strings[] = {
 
 static enum spectre_v2_mitigation spectre_v2_enabled = SPECTRE_V2_NONE;
 
+/* A module has been loaded. Disable reporting that we're good. */
+void disable_retpoline(void)
+{
+	spectre_v2_enabled = SPECTRE_V2_NONE;
+	pr_err("system may be vunerable to spectre\n");
+}
+
+bool retpoline_enabled(void)
+{
+	return spectre_v2_enabled != SPECTRE_V2_NONE;
+}
+
 static void __init spec2_print_if_insecure(const char *reason)
 {
 	if (boot_cpu_has_bug(X86_BUG_SPECTRE_V2))
diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..136ea6cabec6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3020,7 +3020,13 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 				mod->name);
 		add_taint_module(mod, TAINT_OOT_MODULE, LOCKDEP_STILL_OK);
 	}
-
+#ifdef RETPOLINE
+	if (retpoline_enabled() && !get_modinfo(info, "retpoline")) {
+		pr_warn("%s: loading module not compiled with retpoline compiler.\n",
+				mod->name);
+		disable_retpoline();
+	}
+#endif
 	if (get_modinfo(info, "staging")) {
 		add_taint_module(mod, TAINT_CRAP, LOCKDEP_STILL_OK);
 		pr_warn("%s: module is from the staging directory, the quality "
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 98314b400a95..54deaa1066cf 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2165,6 +2165,14 @@ static void add_intree_flag(struct buffer *b, int is_intree)
 		buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n");
 }
 
+/* Cannot check for assembler */
+static void add_retpoline(struct buffer *b)
+{
+	buf_printf(b, "\n#ifdef RETPOLINE\n");
+	buf_printf(b, "MODULE_INFO(retpoline, \"Y\");\n");
+	buf_printf(b, "#endif\n");
+}
+
 static void add_staging_flag(struct buffer *b, const char *name)
 {
 	static const char *staging_dir = "drivers/staging";
@@ -2506,6 +2514,7 @@ int main(int argc, char **argv)
 		err |= check_modname_len(mod);
 		add_header(&buf, mod);
 		add_intree_flag(&buf, !external_module);
+		add_retpoline(&buf);
 		add_staging_flag(&buf, mod->name);
 		err |= add_versions(&buf, mod);
 		add_depends(&buf, mod, modules);

Re: [PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[Randy Dunlap]

On 01/24/2018 10:17 AM, Andi Kleen wrote:

> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index e4dc26185aa7..9064b20473a7 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -93,6 +93,18 @@ static const char *spectre_v2_strings[] = {
>  
>  static enum spectre_v2_mitigation spectre_v2_enabled = SPECTRE_V2_NONE;
>  
> +/* A module has been loaded. Disable reporting that we're good. */
> +void disable_retpoline(void)
> +{
> +	spectre_v2_enabled = SPECTRE_V2_NONE;
> +	pr_err("system may be vunerable to spectre\n");

	                      vulnerable

> +}


-- 
~Randy

Re: [PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[David Woodhouse]

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

On Wed, 2018-01-24 at 10:17 -0800, Andi Kleen wrote:
> +/* A module has been loaded. Disable reporting that we're good. */
> +void disable_retpoline(void)
> +{
> +       spectre_v2_enabled = SPECTRE_V2_NONE;

That wants to be one of the 'MINIMAL' variants.

> +       pr_err("system may be vunerable to spectre\n");
> +}
> +
> +bool retpoline_enabled(void)
> +{
> +       return spectre_v2_enabled != SPECTRE_V2_NONE;

How about checking for X86_FEATURE_RETPOLINE?

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[Thomas Gleixner]

On Wed, 24 Jan 2018, Andi Kleen wrote:

> On Wed, Jan 24, 2018 at 09:00:48AM -0800, Linus Torvalds wrote:
> > On Wed, Jan 24, 2018 at 6:28 AM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > Linus, if there are no objections, can you apply this revert to your
> > > tree now so this doesn't get into 4.15?
> > 
> > Applied.
> 
> So can we get the warning replacement? It would be good to have some
> kind of solution.

I already reviewed that and had comments. So, no. This one is not going to
go anywhere.

Thanks,

	tglx

Re: [PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[Jessica Yu]

+++ Andi Kleen [24/01/18 10:17 -0800]:
>On Wed, Jan 24, 2018 at 09:00:48AM -0800, Linus Torvalds wrote:
>> On Wed, Jan 24, 2018 at 6:28 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> >
>> > Linus, if there are no objections, can you apply this revert to your
>> > tree now so this doesn't get into 4.15?
>>
>> Applied.
>
>So can we get the warning replacement? It would be good to have some
>kind of solution.
>
>-Andi
>
>----
>
>retpoline/module: Warn for missing retpoline in module
>
>There's a risk that a kernel that has full retpoline mitigations
>becomes vulnerable when a module gets loaded that hasn't been
>compiled with the right compiler or the right option.
>
>We cannot fix it, but should at least warn the user when that
>happens.
>
>When the a module hasn't been compiled with a retpoline
>aware compiler, print a warning and change the SPECTRE_V2
>mitigation mode to show the system is vulnerable now.
>
>For modules it is checked at compile time, however it cannot
>check assembler or other non compiled objects used in the module link.
>
>v2: Change warning message
>v3: Port to latest tree
>v4: Remove tainting

So I thought distros wanted the module taint after all, as Greg
mentioned, or is that still overkill? Would the printed warning
be sufficient for the distro folks?

>Cc: jeyu@kernel.org
>Signed-off-by: Andi Kleen <ak@linux.intel.com>
>Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>
>diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>index 9c18da64daa9..ea707c91bd8c 100644
>--- a/arch/x86/include/asm/processor.h
>+++ b/arch/x86/include/asm/processor.h
>@@ -970,4 +970,8 @@ bool xen_set_default_idle(void);
>
> void stop_this_cpu(void *dummy);
> void df_debug(struct pt_regs *regs, long error_code);
>+
>+void disable_retpoline(void);
>+bool retpoline_enabled(void);
>+
> #endif /* _ASM_X86_PROCESSOR_H */
>diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
>index e4dc26185aa7..9064b20473a7 100644
>--- a/arch/x86/kernel/cpu/bugs.c
>+++ b/arch/x86/kernel/cpu/bugs.c
>@@ -93,6 +93,18 @@ static const char *spectre_v2_strings[] = {
>
> static enum spectre_v2_mitigation spectre_v2_enabled = SPECTRE_V2_NONE;
>
>+/* A module has been loaded. Disable reporting that we're good. */
>+void disable_retpoline(void)
>+{
>+	spectre_v2_enabled = SPECTRE_V2_NONE;
>+	pr_err("system may be vunerable to spectre\n");
>+}
>+
>+bool retpoline_enabled(void)
>+{
>+	return spectre_v2_enabled != SPECTRE_V2_NONE;
>+}
>+
> static void __init spec2_print_if_insecure(const char *reason)
> {
> 	if (boot_cpu_has_bug(X86_BUG_SPECTRE_V2))
>diff --git a/kernel/module.c b/kernel/module.c
>index de66ec825992..136ea6cabec6 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3020,7 +3020,13 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
> 				mod->name);
> 		add_taint_module(mod, TAINT_OOT_MODULE, LOCKDEP_STILL_OK);
> 	}
>-
>+#ifdef RETPOLINE
>+	if (retpoline_enabled() && !get_modinfo(info, "retpoline")) {
>+		pr_warn("%s: loading module not compiled with retpoline compiler.\n",
>+				mod->name);
>+		disable_retpoline();
>+	}
>+#endif
> 	if (get_modinfo(info, "staging")) {
> 		add_taint_module(mod, TAINT_CRAP, LOCKDEP_STILL_OK);
> 		pr_warn("%s: module is from the staging directory, the quality "
>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>index 98314b400a95..54deaa1066cf 100644
>--- a/scripts/mod/modpost.c
>+++ b/scripts/mod/modpost.c
>@@ -2165,6 +2165,14 @@ static void add_intree_flag(struct buffer *b, int is_intree)
> 		buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n");
> }
>
>+/* Cannot check for assembler */
>+static void add_retpoline(struct buffer *b)
>+{
>+	buf_printf(b, "\n#ifdef RETPOLINE\n");
>+	buf_printf(b, "MODULE_INFO(retpoline, \"Y\");\n");
>+	buf_printf(b, "#endif\n");
>+}
>+
> static void add_staging_flag(struct buffer *b, const char *name)
> {
> 	static const char *staging_dir = "drivers/staging";
>@@ -2506,6 +2514,7 @@ int main(int argc, char **argv)
> 		err |= check_modname_len(mod);
> 		add_header(&buf, mod);
> 		add_intree_flag(&buf, !external_module);
>+		add_retpoline(&buf);
> 		add_staging_flag(&buf, mod->name);
> 		err |= add_versions(&buf, mod);
> 		add_depends(&buf, mod, modules);
>