From: Andi Kleen <[email protected]>
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: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
Signed-off-by: David Woodhouse <[email protected]>
---
arch/x86/include/asm/processor.h | 4 ++++
arch/x86/kernel/cpu/bugs.c | 12 ++++++++++++
kernel/module.c | 8 +++++++-
scripts/mod/modpost.c | 9 +++++++++
4 files changed, 32 insertions(+), 1 deletion(-)
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);
--
2.14.3
On Fri, 12 Jan 2018, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
> void stop_this_cpu(void *dummy);
> void df_debug(struct pt_regs *regs, long error_code);
> +
> +void disable_retpoline(void);
> +bool retpoline_enabled(void);
Can you please use a consistent name space? retpoline_ ... or such?
> +/* A module has been loaded. Disable reporting that we're good. */
> +void disable_retpoline(void)
> +{
> + spectre_v2_enabled = SPECTRE_V2_NONE;
I really don't like fiddling with that variable. That's just hackery. The
variable reflects the actual enabled mitigation state of the kernel proper.
> + pr_err("system may be vunerable to spectre\n");
> +}
> +
> +bool retpoline_enabled(void)
> +{
> + return spectre_v2_enabled != SPECTRE_V2_NONE;
> +}
That'll break once we get other mitigation variants.
> @@ -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);
> }
> -
These newlines are there to separate stuff for readability sake.
> +#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
This really can be done in a cleaner way.
in linux/module.h
#ifdef RETPOLINE
extern bool retpoline_module_ok(bool has_retpoline);
#else
static inline bool retpoline_module_ok(bool has_retpoline)
{
return true;
}
#endif
static void check_modinfo_retpoline(mod, info)
{
if (retpoline_module_ok(get_modinfo(info, "retpoline")))
return;
pr_warn("%s: loading module not compiled with retpoline compiler.\n",
mod->name);
}
That only needs one function and that one can take care of setting a
variable in the spectre code which then influences the sysfs output.
And that output should not be "Vulnerable" like you force with the hack
above. It actually should tell WHY it is vulnerable despite having had
protection in place before the module was loaded.
Thanks,
tglx
Thanks.
I just sent a v3 that changes the VERMAGIC only, based on Greg's
earlier feedback.
It has the drawbacks that it:
- refuses loading instead of warns
- doesn't stop refusing when the feature is runtime disabled
But it's much simpler, just a few lines of ifdef.
We can either go with the v3, or rework this one into a v4?
-Andi
> I just sent a v3 that changes the VERMAGIC only, based on Greg's
> earlier feedback.
>
> It has the drawbacks that it:
> - refuses loading instead of warns
> - doesn't stop refusing when the feature is runtime disabled
>
> But it's much simpler, just a few lines of ifdef.
I think simple is good at this point
On Tue, 16 Jan 2018, Andi Kleen wrote:
> Thanks.
>
> I just sent a v3 that changes the VERMAGIC only, based on Greg's
> earlier feedback.
>
> It has the drawbacks that it:
> - refuses loading instead of warns
> - doesn't stop refusing when the feature is runtime disabled
>
> But it's much simpler, just a few lines of ifdef.
>
> We can either go with the v3, or rework this one into a v4?
V3 is fine. Not loading is the right thing to do :)
Thanks,
tglx