diff mbox

[v2] cpu: do not leak vulnerabilities to unprivileged users

Message ID 20180126123158.9575-1-Jason@zx2c4.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jason A. Donenfeld Jan. 26, 2018, 12:31 p.m. UTC
While it's public information if the CPU in general has spectre/meltdown
bugs, it probably shouldn't be as globally obvious to all unprivileged
users whether or not the kernel is doing something to mitigate those
bugs. While an attacker can obviously probe and try, there frequently is
a trade-off attackers make of how much probing around they're willing to
do versus the certainty of an attack working, in order to reduce
detection. By making it loud and clear that the kernel _is_ vulnerable,
we're simply aiding the trade-off calculations attackers have to make
when choosing which vectors to target.

So, this patch changes the permissions to 0400 to make the attacker's
job slightly less easy. While we're at it, we clean up the leak in dmesg
too.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
v2 handles dmesg too.

 arch/x86/kernel/cpu/bugs.c | 1 -
 drivers/base/cpu.c         | 6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

Comments

Alan Cox Jan. 26, 2018, 4:43 p.m. UTC | #1
On Fri, 26 Jan 2018 13:31:58 +0100
"Jason A. Donenfeld" <Jason@zx2c4.com> wrote:

> While it's public information if the CPU in general has spectre/meltdown
> bugs, it probably shouldn't be as globally obvious to all unprivileged
> users 

As I replied to you last time you posted this

a) The info is already trivially accessible via /proc/cpuinfo or by
measurement to an attacker
b) Some JIT and other environments need to know

Alan
Boris Lukashev Jan. 26, 2018, 5:23 p.m. UTC | #2
This sort of seems like painting a plate carrier onto a tshirt.
Attackers will know, one way or another, as we have illegitimate and
legitimate vectors for gaining information. Good ecosystem inhabitants
however will be in the dark because this proposes to obfuscate their
legitimate interfaces used to determine execution context.
This might be a better option for namespaces - run an NS with
"hardware spectre mitigation masking" when the consumers in it can
handle that, but leave the rest of the userspace only as broken as it
was before. Without additional protections, bad guys can still glean
the info via leaks and functional testing, but it should provide a
small hurdle without sacrificing JIT ops in the primary NS.

-Boris

On Fri, Jan 26, 2018 at 11:43 AM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> On Fri, 26 Jan 2018 13:31:58 +0100
> "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
>
>> While it's public information if the CPU in general has spectre/meltdown
>> bugs, it probably shouldn't be as globally obvious to all unprivileged
>> users
>
> As I replied to you last time you posted this
>
> a) The info is already trivially accessible via /proc/cpuinfo or by
> measurement to an attacker
> b) Some JIT and other environments need to know
>
> Alan
Jason A. Donenfeld Jan. 26, 2018, 5:47 p.m. UTC | #3
On Fri, Jan 26, 2018 at 5:43 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> a) The info is already trivially accessible via /proc/cpuinfo

No, /proc/cpuinfo shows if the CPU itself has these bugs, but doesn't
show whether or not the kernel has gone to lengths to mitigate these
bugs.

# grep -o 'bugs.*cpu_meltdown' -m1 /proc/cpuinfo
bugs            : cpu_meltdown
# cat /sys/devices/system/cpu/vulnerabilities/meltdown
Mitigation: PTI

> or by measurement to an attacker

Right, so without this, an attacker has to measure. The purpose of
this patchset is to require the attacker to perform an additional
measurement. That seems worthwhile, especially if measurements are or
would ever become non-trivial to make.

> b) Some JIT and other environments need to know

Shouldn't JITs do the best they can with the environment they're in?
And for that, isn't /proc/cpuinfo enough?

Jason
Boris Lukashev Jan. 26, 2018, 6:45 p.m. UTC | #4
On Fri, Jan 26, 2018 at 12:47 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Fri, Jan 26, 2018 at 5:43 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
>> a) The info is already trivially accessible via /proc/cpuinfo
>
> No, /proc/cpuinfo shows if the CPU itself has these bugs, but doesn't
> show whether or not the kernel has gone to lengths to mitigate these
> bugs.
>
> # grep -o 'bugs.*cpu_meltdown' -m1 /proc/cpuinfo
> bugs            : cpu_meltdown
> # cat /sys/devices/system/cpu/vulnerabilities/meltdown
> Mitigation: PTI
>
>> or by measurement to an attacker
>
> Right, so without this, an attacker has to measure. The purpose of
> this patchset is to require the attacker to perform an additional
> measurement. That seems worthwhile, especially if measurements are or
> would ever become non-trivial to make.
>
>> b) Some JIT and other environments need to know
>
> Shouldn't JITs do the best they can with the environment they're in?

In an ideal world, anything using JIT would have a graceful fail-down
to pre-compiled generic paths. As we've seen with PaX' MPROTECT
killing half of the desktop environment along with third party
(browsers/editors/etc) applications, and pretty much every interpreter
or FFI binding-compiled-binary; there's a lot of JIT
compilation/execution out there which doesnt take failure well. I'm
not saying that this would be as detrimental, but in cases where the
JIT gets confused and does "the wrong thing," all sorts of undefined
behaviors might occur ranging from poorly performing execution to
security concerns and crashes. _If_ such confusion is possible, is the
benefit of a layer of concealment (as opposed to cover) worth the
potential effort in tracking down problems which could be caused by
this? How likely are users to bypass the constraint by running as root
and expose their system to more threat?

> And for that, isn't /proc/cpuinfo enough?
>
> Jason
Alan Cox Jan. 26, 2018, 7:07 p.m. UTC | #5
On Fri, 26 Jan 2018 18:47:28 +0100
"Jason A. Donenfeld" <Jason@zx2c4.com> wrote:

> On Fri, Jan 26, 2018 at 5:43 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> > a) The info is already trivially accessible via /proc/cpuinfo  
> 
> No, /proc/cpuinfo shows if the CPU itself has these bugs, but doesn't
> show whether or not the kernel has gone to lengths to mitigate these
> bugs.

It tells you immediately what microcode, what hardware properties. A few
user space instructions later and you know the rest.

> Right, so without this, an attacker has to measure. The purpose of
> this patchset is to require the attacker to perform an additional
> measurement. That seems worthwhile, especially if measurements are or
> would ever become non-trivial to make.

You mean 'I'm magically going to make it secure because the attacker
won't be smart enough to paste in a function from gitub' ?

I don't buy it. Most attack tools are automated, they will contain the
needed code.

> > b) Some JIT and other environments need to know  
> 
> Shouldn't JITs do the best they can with the environment they're in?
> And for that, isn't /proc/cpuinfo enough?

No

The JIT needs to know whether the mitigations are present because it
needs to JIT very different code if it is responsible for generating
things like lfence and and/mask references or can assume the CPU is
covering some or part of it.

Likewise you can imagine other code using those files in order to figure
out how to self patch / which library to load for performance critical
stuff.

Alan
diff mbox

Patch

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 390b3dc3d438..e512ae82f201 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -230,7 +230,6 @@  static void __init spectre_v2_select_mitigation(void)
 	}
 
 	spectre_v2_enabled = mode;
-	pr_info("%s\n", spectre_v2_strings[mode]);
 
 	/*
 	 * If neither SMEP or KPTI are available, there is a risk of
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index d99038487a0d..a3a8e008f957 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -531,9 +531,9 @@  ssize_t __weak cpu_show_spectre_v2(struct device *dev,
 	return sprintf(buf, "Not affected\n");
 }
 
-static DEVICE_ATTR(meltdown, 0444, cpu_show_meltdown, NULL);
-static DEVICE_ATTR(spectre_v1, 0444, cpu_show_spectre_v1, NULL);
-static DEVICE_ATTR(spectre_v2, 0444, cpu_show_spectre_v2, NULL);
+static DEVICE_ATTR(meltdown, 0400, cpu_show_meltdown, NULL);
+static DEVICE_ATTR(spectre_v1, 0400, cpu_show_spectre_v1, NULL);
+static DEVICE_ATTR(spectre_v2, 0400, cpu_show_spectre_v2, NULL);
 
 static struct attribute *cpu_root_vulnerabilities_attrs[] = {
 	&dev_attr_meltdown.attr,