Re: [patch V2 1/2] sysfs/cpu: Add vulnerability folder

[Andrea Arcangeli <aarcange@redhat.com>]

Hello,

On Sun, Jan 07, 2018 at 10:48:00PM +0100, Thomas Gleixner wrote:
> +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);

This sysfs feature implemented as above is weakening kernel security,
it should be 0400 above.

It doesn't make sense to expose to luser when a software fix (or even
only a software mitigation) has been disabled at build time to gain
all performance back (see CONFIG_RETPOLINE=n config option).

$ cat /boot/kernel-`uname -r`
cat: /boot/kernel-4.15.0-rc9+: Permission denied
$ cat /lib/modules/`uname -r`/kernel/arch/x86/kvm/kvm.ko 
cat: /lib/modules/4.15.0-rc9+/kernel/arch/x86/kvm/kvm.ko: Permission denied
$ dmesg
dmesg: read kernel buffer failed: Operation not permitted

Noticing when cr3 is flipped in kernel/exit is easy, but noticing when
the syscall table or the whole kernel has been built with retpolines
is not trivial to detect. Same for variant#1.

Exploiting spectre variant#2 for an attacker is not without risk of
being detected while the setup is being mounted, as the CPU load would
spike for hours.

I may notice if a random background network daemon suddenly starts
running at 100% CPU load for hours (especially on mobile devices that
would be physically noticeable).

Containers shouldn't have sysfs and you can workaround the above if
you run all network daemons behind mount namespaces, but in general
leaving this directory readable by luser is weaking security because
it exposes when mounting a variant#2 attack can succeed.

It even tells when it is worth to focus on the syscall_table indirect
call or if the attack needs to dig deeper because asm retpolines were
used, but the kernel was built with a gcc without retpolines.

The only case where the above isn't weakening security is when the
full fix is on for all the variants is enabled (and variant#1 for now
just shows vulnerable..).

For the same reasons the whole directory, not just the files, should be
0500, especially if this would be used for any other equivalent issue
in the future and it won't stick to these 3 files, I didn't implement
that yet, because it's less urgent if nobody adds any more files soon.

>From 578b411c8dcb1435dd1f94a6cd062f4eedb70fb5 Mon Sep 17 00:00:00 2001
From: Andrea Arcangeli <aarcange@redhat.com>
Date: Wed, 24 Jan 2018 19:19:36 +0100
Subject: [PATCH 1/1] x86/spectre/meltdown: avoid the vulnerability directory
 to weaken kernel security

If any of the fixes is disabled to gain some performance back at
runtime or build time, should not be exposed to unprivileged userland.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 drivers/base/cpu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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,