[PATCH] retpoline/module: Taint kernel for missing retpoline in module

[PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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.

Add a flag to each module if it has been compiled with RETPOLINE

When the a module hasn't been compiled with a retpoline
aware compiler, print a warning and set a taint flag.

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

Due to lack of better letter it uses taint option 'Z'

We only set the taint flag for incorrectly compiled modules
now, not for the main kernel, which already has other
report mechanisms.

Also make sure to report vulnerable for spectre if such a module
has been loaded.

v2: Change warning message
v3: Port to latest tree
Cc: jeyu@kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 Documentation/admin-guide/tainted-kernels.rst |  3 +++
 arch/x86/include/asm/processor.h              |  4 ++++
 arch/x86/kernel/cpu/bugs.c                    | 12 ++++++++++++
 include/linux/kernel.h                        |  3 ++-
 kernel/module.c                               | 11 ++++++++++-
 kernel/panic.c                                |  2 ++
 scripts/mod/modpost.c                         |  9 +++++++++
 7 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
index 1df03b5cb02f..adf0cb0dcf9b 100644
--- a/Documentation/admin-guide/tainted-kernels.rst
+++ b/Documentation/admin-guide/tainted-kernels.rst
@@ -52,6 +52,9 @@ characters, each representing a particular tainted value.
 
  16) ``K`` if the kernel has been live patched.
 
+ 17) ``Z`` if a module hasn't been compiled with
+     a retpoline aware compiler and may be vulnerable to data leaks.
+
 The primary reason for the **'Tainted: '** string is to tell kernel
 debuggers if this is a clean kernel or if anything unusual has
 occurred.  Tainting is permanent: even if an offending module is
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/include/linux/kernel.h b/include/linux/kernel.h
index 4b484ab9e163..b0e0cbf5ee9a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -549,7 +549,8 @@ extern enum system_states {
 #define TAINT_UNSIGNED_MODULE		13
 #define TAINT_SOFTLOCKUP		14
 #define TAINT_LIVEPATCH			15
-#define TAINT_FLAGS_COUNT		16
+#define TAINT_NO_RETPOLINE		16
+#define TAINT_FLAGS_COUNT		17
 
 struct taint_flag {
 	char c_true;	/* character printed when tainted */
diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..f114eee110d4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3020,7 +3020,16 @@ 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")) {
+		if (!test_taint(TAINT_NO_RETPOLINE)) {
+			pr_warn("%s: loading module not compiled with retpoline compiler.\n",
+				mod->name);
+		}
+		add_taint_module(mod, TAINT_NO_RETPOLINE, LOCKDEP_STILL_OK);
+		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/kernel/panic.c b/kernel/panic.c
index bdd18afa19a4..f8629eb1849d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -322,6 +322,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 	{ 'E', ' ', true },	/* TAINT_UNSIGNED_MODULE */
 	{ 'L', ' ', false },	/* TAINT_SOFTLOCKUP */
 	{ 'K', ' ', true },	/* TAINT_LIVEPATCH */
+	{ 'Z', ' ', true },	/* TAINT_NO_RETPOLINE */
 };
 
 /**
@@ -343,6 +344,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
  *  'E' - Unsigned module has been loaded.
  *  'L' - A soft lockup has previously occurred.
  *  'K' - Kernel has been live patched.
+ *  'Z' - Module with no retpoline has been loaded
  *
  *	The string is overwritten by the next call to print_tainted().
  */
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

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[David Woodhouse]

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

On Fri, 2018-01-12 at 09:55 -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> 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.
> 
> Add a flag to each module if it has been compiled with RETPOLINE
> 
> When the a module hasn't been compiled with a retpoline
> aware compiler, print a warning and set a taint flag.
> 
> For modules it is checked at compile time, however it cannot
> check assembler or other non compiled objects used in the module link.
> 
> Due to lack of better letter it uses taint option 'Z'
> 
> We only set the taint flag for incorrectly compiled modules
> now, not for the main kernel, which already has other
> report mechanisms.
> 
> Also make sure to report vulnerable for spectre if such a module
> has been loaded.

Thanks for reviving this; it got dropped partly because it has
conflicts between the tip/x86/pti tree and Linus' 4.15-rc.

The other reason for dropping it was because I think we probably want
to revisit this whole thing once we have all the mitigations in place.

It doesn't make a lot of sense to have a taint flag for a *partial*
retpoline, but not in the case that we have *no* mitigation in place.

So maybe we should drop the taint part, and just make the kernel report
that it is (partially) vulnerable to Spectre V2, just as in the
CONFIG_RETPOLINE && !RETPOLINE case?

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

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Andi Kleen]

> It doesn't make a lot of sense to have a taint flag for a *partial*
> retpoline, but not in the case that we have *no* mitigation in place.

The only thing that makes sense checking for is the C compiler
option. Everything else which needs manual changes we cannot check.

So even if we add more things I don't think this particular
check will change.


> So maybe we should drop the taint part, and just make the kernel report
> that it is (partially) vulnerable to Spectre V2, just as in the
> CONFIG_RETPOLINE && !RETPOLINE case?

Ok I can drop the taint part. The reporting is already implemented.

-Andi

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Greg KH]

On Fri, Jan 12, 2018 at 09:55:07AM -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> 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.
> 
> Add a flag to each module if it has been compiled with RETPOLINE
> 
> When the a module hasn't been compiled with a retpoline
> aware compiler, print a warning and set a taint flag.

Isn't that caught by the "build with a different compiler/version" check
that we have?  Or used to have?  If not, can't we just make it into that
type of check to catch this type of problem no matter what type of
feature/option it is trying to catch?

thanks,

greg k-h

RE: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Van De Ven, Arjan]

> > When the a module hasn't been compiled with a retpoline
> > aware compiler, print a warning and set a taint flag.
> 
> Isn't that caught by the "build with a different compiler/version" check
> that we have?  Or used to have?  If not, can't we just make it into that
> type of check to catch this type of problem no matter what type of
> feature/option it is trying to catch?


making retpoline part of the modversion hash thingy could make sense.

but I kinda feel this is a bit overkill; it's not a function issue if you get this wrong, and if you run an ancient or weird out of tree module there's a real chance you have
other security fun as well ;-)

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Andi Kleen]

> > When the a module hasn't been compiled with a retpoline
> > aware compiler, print a warning and set a taint flag.
> 
> Isn't that caught by the "build with a different compiler/version" check
> that we have?  Or used to have?  If not, can't we just make it into that

- the compiler version number may not change if a distribution backports
the gcc changes for the new flag
- the module might be using a custom make file that does not correctly
set the flag, even if the compiler supports it

> type of check to catch this type of problem no matter what type of
> feature/option it is trying to catch?

I suspect that would be far more complicated. Also what's the point
of putting this information into every symbol? Once per module 
is good enough.

We already have similar checks for staging etc.

-Andi

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Greg KH]

On Sat, Jan 13, 2018 at 06:53:00AM -0800, Andi Kleen wrote:
> > > When the a module hasn't been compiled with a retpoline
> > > aware compiler, print a warning and set a taint flag.
> > 
> > Isn't that caught by the "build with a different compiler/version" check
> > that we have?  Or used to have?  If not, can't we just make it into that
> 
> - the compiler version number may not change if a distribution backports
> the gcc changes for the new flag
> - the module might be using a custom make file that does not correctly
> set the flag, even if the compiler supports it
> 
> > type of check to catch this type of problem no matter what type of
> > feature/option it is trying to catch?
> 
> I suspect that would be far more complicated.

Really?  As Arjan points out, just mix it into the modversion symbol
generation, that should cause it to be caught properly and trivially.

> Also what's the point of putting this information into every symbol?

It makes it easy to check :)

> Once per module is good enough.
> 
> We already have similar checks for staging etc.

Sure, but this is more of a "Hey, your version of GCC is doing something
different than what you built the kernel with, watch out!" which is much
more generic and good to know.  A whole taint for one CPU bug type seems
overkill to me.

thanks,

greg k-h

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Greg KH]

On Sat, Jan 13, 2018 at 02:38:51PM +0000, Van De Ven, Arjan wrote:
> > > When the a module hasn't been compiled with a retpoline
> > > aware compiler, print a warning and set a taint flag.
> > 
> > Isn't that caught by the "build with a different compiler/version" check
> > that we have?  Or used to have?  If not, can't we just make it into that
> > type of check to catch this type of problem no matter what type of
> > feature/option it is trying to catch?
> 
> 
> making retpoline part of the modversion hash thingy could make sense.
> 
> but I kinda feel this is a bit overkill; it's not a function issue if
> you get this wrong, and if you run an ancient or weird out of tree
> module there's a real chance you have other security fun as well ;-)

Sure, but take pity on the crazy distro developers who have to support
crap like this.  They really want to know if a module is built
differently from the kernel, to force the user to know they are on their
own.

modversion seems like a trivial thing to mix this into, and solves the
distro issue at the same time.

thanks,

greg k-h

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Andi Kleen]

> > Also what's the point of putting this information into every symbol?
> 
> It makes it easy to check :)

Easier than nm?

Per symbol still doesn't make any sense to me.

> 
> > Once per module is good enough.
> > 
> > We already have similar checks for staging etc.
> 
> Sure, but this is more of a "Hey, your version of GCC is doing something
> different than what you built the kernel with, watch out!" which is much
> more generic and good to know.  A whole taint for one CPU bug type seems
> overkill to me.

I removed the taint in version 2, posted yesterday. It now just prints
the warning and resets the vulnerability reporting in sysfs.

-Andi

RE: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[David Laight]

From: Andi Kleen
> Sent: 12 January 2018 17:55
> 
> 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.
> 
> Add a flag to each module if it has been compiled with RETPOLINE
> 
> When the a module hasn't been compiled with a retpoline
> aware compiler, print a warning and set a taint flag.
> 
> For modules it is checked at compile time, however it cannot
> check assembler or other non compiled objects used in the module link.

It is not unlikely that most of a module's code is released as a
binary 'blob', with only the part that needs to match the kernel ABI
compiled on the target system.
(This allows a single binary driver be loaded onto different Linux
kernel versions.)
Such code is unlikely to be compiled with the latest compiler.
Indeed, doing so may stop the module loading on old kernels.

	David

RE: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Van De Ven, Arjan]

> > For modules it is checked at compile time, however it cannot
> > check assembler or other non compiled objects used in the module link.
> 
> It is not unlikely that most of a module's code is released as a
> binary 'blob', with only the part that needs to match the kernel ABI
> compiled on the target system.
> (This allows a single binary driver be loaded onto different Linux
> kernel versions.)

binary what? ;-)

retpoline (or lack thereof) is part of the kernel ABI at this point.... 

now clearly security does not matter for this (not unlikely that you have more severe issues) but warning of a mismatch is not a bad thing so that you at least know.

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[David Woodhouse]

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

On Mon, 2018-01-15 at 12:53 +0000, Van De Ven, Arjan wrote:
> 
> binary what? ;-)
> 
> retpoline (or lack thereof) is part of the kernel ABI at this point.... 

Strictly speaking, only lack thereof.

If you build the kernel without CONFIG_RETPOLINE, you can't build
modules with retpoline and then expect them to load — because the thunk
symbols aren't exported.

If you build the kernel with the retpoline though, you *can*
successfully load modules which aren't built with retpoline.

It's not even that unlikely, if the kernel engineers are ensuring they
have the latest unreleased toolchain for kernel builds, but you're
using out-of-tree modules built separately or even on the target system
using DKMS, and you *haven't* got the shiny new compiler.

That's what Andi's attempting to protect against.

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

RE: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[David Laight]

From: David Woodhouse
> Sent: 15 January 2018 13:01
> On Mon, 2018-01-15 at 12:53 +0000, Van De Ven, Arjan wrote:
> >
> > binary what? ;-)
> >
> > retpoline (or lack thereof) is part of the kernel ABI at this point....
> 
> Strictly speaking, only lack thereof.
> 
> If you build the kernel without CONFIG_RETPOLINE, you can't build
> modules with retpoline and then expect them to load — because the thunk
> symbols aren't exported.
> 
> If you build the kernel with the retpoline though, you *can*
> successfully load modules which aren't built with retpoline.
> 
> It's not even that unlikely, if the kernel engineers are ensuring they
> have the latest unreleased toolchain for kernel builds, but you're
> using out-of-tree modules built separately or even on the target system
> using DKMS, and you *haven't* got the shiny new compiler.

More likely you have got the 'shiny new compiler', but the kernel the
code is loading on is old and archaic.

We haven't yet managed to persuade all our customers to move from
2.6.18 kernels (think that is a supported RHEL release).

> That's what Andi's attempting to protect against.

Hopefully he'll only mark things 'tainted'.

	David

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Andi Kleen]

> It is not unlikely that most of a module's code is released as a
> binary 'blob', with only the part that needs to match the kernel ABI
> compiled on the target system.

Yes that is true. However such blob build systems are usually
done with custom Makefiles, not Kbuild, and those Makefiles don't set
-DRETPOLINE, so it would still be caught.

Now if someone sets -DRETPOLINE on a blob build it wouldn't warn,
but that would be actively malicious and there's no way to protect
against that.

It's merely aimed at detecting mistakes.

-Andi

Re: [PATCH] retpoline/module: Taint kernel for missing retpoline in module

[Andi Kleen]

On Sat, Jan 13, 2018 at 04:36:44PM +0100, Greg KH wrote:
> On Sat, Jan 13, 2018 at 06:53:00AM -0800, Andi Kleen wrote:
> > > > When the a module hasn't been compiled with a retpoline
> > > > aware compiler, print a warning and set a taint flag.
> > > 
> > > Isn't that caught by the "build with a different compiler/version" check
> > > that we have?  Or used to have?  If not, can't we just make it into that
> > 
> > - the compiler version number may not change if a distribution backports
> > the gcc changes for the new flag
> > - the module might be using a custom make file that does not correctly
> > set the flag, even if the compiler supports it
> > 
> > > type of check to catch this type of problem no matter what type of
> > > feature/option it is trying to catch?
> > 
> > I suspect that would be far more complicated.
> 
> Really?  As Arjan points out, just mix it into the modversion symbol
> generation, that should cause it to be caught properly and trivially.

It seems it's more obvious to put it into VERMAGIC. That should
be good enough too?

This gives it an actual string that can be printed.

Otherwise there won't be a clear error message on what's wrong.

-Andi