Do not allow MSR or Embedded Controller writes from userspace in secure boot case

Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Thomas Renninger]

Hi,

I have seen some patches in this area and I wonder whether MSR and EC write accesses from userspace got closed already.

If you can point me to a latest git tree where such patches get applied, I can double check before sending such stuff.
Afaik Matthew collects such patches, but I could not find a related git tree on git.kernel.org.

Anyway, if these make sense, feel free to queue them in an appropriate tree/branch.

   Thomas

[PATCH 1/2] ACPI ec_sys: Do not allow write access to EC in secure boot mode

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org>
---
 drivers/acpi/ec_sys.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
index 7586544..51dea3b 100644
--- a/drivers/acpi/ec_sys.c
+++ b/drivers/acpi/ec_sys.c
@@ -12,6 +12,8 @@
 #include <linux/acpi.h>
 #include <linux/debugfs.h>
 #include <linux/module.h>
+#include <linux/capability.h>
+
 #include "internal.h"
 
 MODULE_AUTHOR("Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org>");
@@ -68,6 +70,9 @@ static ssize_t acpi_ec_write_io(struct file *f, const char __user *buf,
 	u8 *data = (u8 *) buf;
 	int err = 0;
 
+	if (!capable(CAP_COMPROMISE_KERNEL))
+		return -EPERM;
+
 	if (*off >= EC_SPACE_SIZE)
 		return 0;
 	if (*off + count >= EC_SPACE_SIZE) {
@@ -121,8 +126,13 @@ int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned int ec_device_count)
 				 (u32 *)&first_ec->global_lock))
 		goto error;
 
-	if (write_support)
-		mode = 0600;
+	if (write_support) {
+		if (!capable(CAP_COMPROMISE_KERNEL))
+			pr_err("No write access to EC\n");
+		else
+			mode = 0600;
+	}
+
 	if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops))
 		goto error;
 
-- 
1.7.6.1

[PATCH 2/2] X86 msr: Do not allow MSR writes in secure boot mode

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org>
---
 arch/x86/kernel/msr.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index a7c5661..3de9128 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -37,6 +37,7 @@
 #include <linux/notifier.h>
 #include <linux/uaccess.h>
 #include <linux/gfp.h>
+#include <linux/capability.h>
 
 #include <asm/processor.h>
 #include <asm/msr.h>
@@ -103,6 +104,9 @@ static ssize_t msr_write(struct file *file, const char __user *buf,
 	int err = 0;
 	ssize_t bytes = 0;
 
+	if (!capable(CAP_COMPROMISE_KERNEL))
+		return -EPERM;
+
 	if (count % 8)
 		return -EINVAL;	/* Invalid chunk size */
 
-- 
1.7.6.1

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Matthew Garrett]

Is there a case where modifying MSRs or EC registers can cause arbitrary 
code execution?

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[H. Peter Anvin]

On 11/07/2012 10:54 PM, Matthew Garrett wrote:
> Is there a case where modifying MSRs or EC registers can cause arbitrary 
> code execution?

It definitely can't be ruled out.

	-hpa

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[H. Peter Anvin]

On 11/07/2012 10:54 PM, Matthew Garrett wrote:
> Is there a case where modifying MSRs or EC registers can cause arbitrary 
> code execution?

For MSRs we could have a whitelist of permitted MSRs, but allowing
general MSR access... no.

	-hpa

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Alan Cox]

On Wed, 07 Nov 2012 23:51:06 +0100
"H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:

> On 11/07/2012 10:54 PM, Matthew Garrett wrote:
> > Is there a case where modifying MSRs or EC registers can cause arbitrary 
> > code execution?
> 
> For MSRs we could have a whitelist of permitted MSRs, but allowing
> general MSR access... no.

Far safer to just block it and expect people to use MSRs accessed via
drivers that provide functionality. Otherwise your blacklist in some
cases will get fiendishly complicated and sometimes depend upon the exact
chip and the values in other MSRs as to whether they are safe.

Alan

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Thomas Renninger]

On Wednesday, November 07, 2012 11:51:06 PM H. Peter Anvin wrote:
> On 11/07/2012 10:54 PM, Matthew Garrett wrote:
> > Is there a case where modifying MSRs or EC registers can cause
> > arbitrary code execution?
> 
> For MSRs we could have a whitelist of permitted MSRs, but allowing
> general MSR access... no.

BTW: Who decides what is allowed and what is not?

1) hpa?
2) Intel
3) The efi list?
4) The spec?
5) Windows when they threat distributions to revoke their
   key if they do not do this and that?

I guess it should be the spec. I haven't read the details, but
when even Matthew is not sure, it sounds as if this is phrased
rather imprecise. And as Windows is afaik the central key authority
they can enforce their interpretation of the spec for Linux as well?

An example:

I have seen (shortened) a patch like this:

Secure boot: Add a dummy kernel parameter that will switch on Secure Boot mode
+__setup("secureboot_enable=", secureboot_enable_opt);

This is to enforce secure boot restrictions even if
HW does not support UEFI version 2.3.1 (or whatsoever).

I like to have this boot parameter to also work the
other way around:
secureboot_enable=no
and let all secure boot things fall off, only set a
TAINT_INSECURE_BOOT_EVEN_BIOS_REQUESTED_SECURE_BOOT

Can SUSE sign this kernel without fearing to get the key revoked
from Windows?
Can this exist in the mainline kernel?

   Thomas

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Matthew Garrett]

On Thu, Nov 08, 2012 at 03:38:33PM +0100, Thomas Renninger wrote:

> BTW: Who decides what is allowed and what is not?

Tree maintainers.

> I guess it should be the spec. I haven't read the details, but
> when even Matthew is not sure, it sounds as if this is phrased
> rather imprecise. And as Windows is afaik the central key authority
> they can enforce their interpretation of the spec for Linux as well?

The spec is purely mechanism, not policy. Policy is up to the OS 
vendors.

> I like to have this boot parameter to also work the
> other way around:
> secureboot_enable=no
> and let all secure boot things fall off, only set a
> TAINT_INSECURE_BOOT_EVEN_BIOS_REQUESTED_SECURE_BOOT
> 
> Can SUSE sign this kernel without fearing to get the key revoked
> from Windows?

If anyone used that kernel to attack Windows, the signature would get 
revoked.

> Can this exist in the mainline kernel?

Sure, but vendors might want to patch it out, depending on how paranoid 
they are.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Shea Levy]

On 11/08/2012 09:41 AM, Matthew Garrett wrote:
> On Thu, Nov 08, 2012 at 03:38:33PM +0100, Thomas Renninger wrote:
>
>> BTW: Who decides what is allowed and what is not?
> Tree maintainers.
>
>> I guess it should be the spec. I haven't read the details, but
>> when even Matthew is not sure, it sounds as if this is phrased
>> rather imprecise. And as Windows is afaik the central key authority
>> they can enforce their interpretation of the spec for Linux as well?
> The spec is purely mechanism, not policy. Policy is up to the OS
> vendors.
>
>> I like to have this boot parameter to also work the
>> other way around:
>> secureboot_enable=no
>> and let all secure boot things fall off, only set a
>> TAINT_INSECURE_BOOT_EVEN_BIOS_REQUESTED_SECURE_BOOT
>>
>> Can SUSE sign this kernel without fearing to get the key revoked
>> from Windows?
> If anyone used that kernel to attack Windows, the signature would get
> revoked.
>
>> Can this exist in the mainline kernel?
> Sure, but vendors might want to patch it out, depending on how paranoid
> they are.
>

How is secureboot_enable=no ok? Unless we're disabling efivarfs in 
secureboot mode root can change the kernel command line.

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Matthew Garrett]

On Thu, Nov 08, 2012 at 09:44:11AM -0500, Shea Levy wrote:

> How is secureboot_enable=no ok? Unless we're disabling efivarfs in
> secureboot mode root can change the kernel command line.

What do you mean by "ok"? Ubuntu ship a signed kernel without requiring 
signed modules, so any in-kernel protections can be trivially 
circumvented. They've made that decision based on a risk/benefit 
analysis. Every vendor is going to have to make their own analysis, and 
it's not guaranteed that the upstream kernel is going to precisely match 
any of them.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[H. Peter Anvin]

We should only whitelist MSRs that are needed by some widely used things... no whitelist is the easiest.

Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org> wrote:

>On Wednesday, November 07, 2012 11:51:06 PM H. Peter Anvin wrote:
>> On 11/07/2012 10:54 PM, Matthew Garrett wrote:
>> > Is there a case where modifying MSRs or EC registers can cause
>> > arbitrary code execution?
>> 
>> For MSRs we could have a whitelist of permitted MSRs, but allowing
>> general MSR access... no.
>
>BTW: Who decides what is allowed and what is not?
>
>1) hpa?
>2) Intel
>3) The efi list?
>4) The spec?
>5) Windows when they threat distributions to revoke their
>   key if they do not do this and that?
>
>I guess it should be the spec. I haven't read the details, but
>when even Matthew is not sure, it sounds as if this is phrased
>rather imprecise. And as Windows is afaik the central key authority
>they can enforce their interpretation of the spec for Linux as well?
>
>An example:
>
>I have seen (shortened) a patch like this:
>
>Secure boot: Add a dummy kernel parameter that will switch on Secure
>Boot mode
>+__setup("secureboot_enable=", secureboot_enable_opt);
>
>This is to enforce secure boot restrictions even if
>HW does not support UEFI version 2.3.1 (or whatsoever).
>
>I like to have this boot parameter to also work the
>other way around:
>secureboot_enable=no
>and let all secure boot things fall off, only set a
>TAINT_INSECURE_BOOT_EVEN_BIOS_REQUESTED_SECURE_BOOT
>
>Can SUSE sign this kernel without fearing to get the key revoked
>from Windows?
>Can this exist in the mainline kernel?
>
>   Thomas

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Thomas Renninger]

On Wednesday, November 07, 2012 09:54:03 PM Matthew Garrett wrote:
> Is there a case where modifying MSRs or EC registers can cause arbitrary
> code execution?

Ok, I am not familiar enough with this secure stuff.
Theoretically writing EC registers could be used to trick ACPI
code and change the way it is processed by inspecting ACPI
code for bad EC register return values.

Similar for MSR, the kernel could be (not directly) influenced
by setting MSR registers in a way it does not expect them to be.

I expect it's easy to get the system totally stalled/hang/rebooted
with bad MSR writes which I thought should be forbidden for
userspace (even for root...) in secure boot mode.

   Thomas

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Matthew Garrett]

On Thu, Nov 08, 2012 at 10:40:33AM +0100, Thomas Renninger wrote:
> On Wednesday, November 07, 2012 09:54:03 PM Matthew Garrett wrote:
> > Is there a case where modifying MSRs or EC registers can cause arbitrary
> > code execution?
> 
> Ok, I am not familiar enough with this secure stuff.
> Theoretically writing EC registers could be used to trick ACPI
> code and change the way it is processed by inspecting ACPI
> code for bad EC register return values.

I'd prefer to see an actual example before worrying too much about this.

> Similar for MSR, the kernel could be (not directly) influenced
> by setting MSR registers in a way it does not expect them to be.

Again, I'd like to see an example of arbitrary code execution.

> I expect it's easy to get the system totally stalled/hang/rebooted
> with bad MSR writes which I thought should be forbidden for
> userspace (even for root...) in secure boot mode.

root can call halt, which is a trivial DoS. It's not worth worrying 
about people wedging the system.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Alan Cox]

On Thu, 8 Nov 2012 14:39:19 +0000
Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org> wrote:

> On Thu, Nov 08, 2012 at 10:40:33AM +0100, Thomas Renninger wrote:
> > On Wednesday, November 07, 2012 09:54:03 PM Matthew Garrett wrote:
> > > Is there a case where modifying MSRs or EC registers can cause arbitrary
> > > code execution?
> > 
> > Ok, I am not familiar enough with this secure stuff.
> > Theoretically writing EC registers could be used to trick ACPI
> > code and change the way it is processed by inspecting ACPI
> > code for bad EC register return values.
> 
> I'd prefer to see an actual example before worrying too much about this.
> 
> > Similar for MSR, the kernel could be (not directly) influenced
> > by setting MSR registers in a way it does not expect them to be.
> 
> Again, I'd like to see an example of arbitrary code execution.

I can think of a few. However that also shows up the lack of
CAP_SYS_RAWIO checking on this interface so I think we need to get that
fixed before posting the obvious ones as its otherwise a way to get from
DAC to RAWIO.

Alan

Re: Do not allow MSR or Embedded Controller writes from userspace in secure boot case

[Alan Cox]

On Wed,  7 Nov 2012 22:28:17 +0100
Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org> wrote:

> Hi,
> 
> I have seen some patches in this area and I wonder whether MSR and EC write accesses from userspace got closed already.

You need to cover read accesses as well I suspect to be completely
paranoid safe. I would also look at MTRRs because mis-setting MTRRs
allows you to get firmware to do interesting things in certain situations
because the commands being issued to stuff like the GPU may get corrupted.

Alan

[RFC] [PATCH] X86 MSR read whitelist

[Thomas Renninger]

On Thursday, November 08, 2012 12:27:22 AM Alan Cox wrote:
> On Wed,  7 Nov 2012 22:28:17 +0100
> Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org> wrote:
> 
> > Hi,
> > 
> > I have seen some patches in this area and I wonder whether MSR and EC
> > write accesses from userspace got closed already.
> 
> You need to cover read accesses as well I suspect to be completely
> paranoid safe.
I feared that this could be asked for...
While I am not aware of any userspace tools urgently needing MSR
write access, there are several userspace tools reading MSRs.

What exactly could go wrong with which MSR read?
I suggest to go for:
  - deny writing MSRs from userspace
  - allow reading MSRs from userspace
(via msr driver) for now.
In case there is urgent need for a whitelist, an implementation
example (some defines do not exist yet) is pasted in the end for
discussion (or picking up). Be careful, not well tested.

I just picked some MSRs I found in turbostat and cpupower.
Len: It would be great if you could contribute to cpupower.
It can do exactly the same than turbostat (but for all archs)
and the fixups you send for turbostat are very easy to integrate
into cpupower as well.

> I would also look at MTRRs because mis-setting MTRRs
> allows you to get firmware to do interesting things in certain
> situations because the commands being issued to stuff like the GPU may
> get corrupted.
I am not aware of a mtrr interface to userspace.

   Thomas


X86 msr: Secure boot restrict MSR reads by whitelisting safe ones

Signed-off-by: Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org>
---
 arch/x86/kernel/msr.c           |    9 ++++-
 arch/x86/kernel/msr_whitelist.h |   75 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index 3de9128..c7a6bb2 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -42,6 +42,8 @@
 #include <asm/processor.h>
 #include <asm/msr.h>
 
+#include "msr_whitelist.h"
+
 static struct class *msr_class;
 
 static loff_t msr_seek(struct file *file, loff_t offset, int orig)
@@ -76,8 +78,11 @@ static ssize_t msr_read(struct file *file, char __user *buf,
 	int err = 0;
 	ssize_t bytes = 0;
 
-	if (count % 8)
-		return -EINVAL;	/* Invalid chunk size */
+	if (count % 8 || reg == 0)
+		return -EINVAL;	/* Invalid chunk size or zero MSR */
+
+	if (!msr_is_allowed(reg, cpu))
+		return -EPERM;
 
 	for (; count; count -= 8) {
 		err = rdmsr_safe_on_cpu(cpu, reg, &data[0], &data[1]);
diff --git a/arch/x86/kernel/msr_whitelist.h b/arch/x86/kernel/msr_whitelist.h
new file mode 100644
index 0000000..940e05a
--- /dev/null
+++ b/arch/x86/kernel/msr_whitelist.h
@@ -0,0 +1,75 @@
+#ifndef X86_SECURE_MSR_WHITELIST_H
+#define X86_SECURE_MSR_WHITELIST_H
+
+#ifdef X86_SECURE_BOOT /* Does this exist? */
+
+#include <linux/capability.h>
+
+static u32 msr_generic_wl[] =
+	{
+		0x10, /* MSR_TSC */
+		0xe8, /* MSR_APERF */
+		0xe7, /* MSR_MPERF */
+		0x10, /* MSR_TSC */
+		0x34, /* MSR_SMI_COUNT */
+		0
+	};
+
+static u32 msr_amd_wl[] =
+	{
+		0xc0010063, /* MSR_AMD_PSTATE_STATUS */
+		0xc0010064, /* MSR_AMD_PSTATE */
+		0xc0010061, /* MSR_AMD_PSTATE_LIMIT */
+		0xc0010015, /* MSR_AMD_HWCR */
+		0
+	};
+
+static u32 msr_intel_wl[] =
+	{
+		0x198, /* MSR_IA32_PERF_STATUS */
+		0x1a0, /* MSR_IA32_MISC_ENABLES */
+		0x1b0, /* MSR_IA32_ENERGY_PERF_BIAS */
+		0xce,  /* MSR_NEHALEM_PLATFORM_INFO */
+		0x1ad, /* MSR_NEHALEM_TURBO_RATIO_LIMIT */
+		0x3f8, /* MSR_PKG_C3_RESIDENCY */
+		0x3f9, /* MSR_PKG_C6_RESIDENCY */
+		0x3fc, /* MSR_CORE_C3_RESIDENCY */
+		0x3fd, /* MSR_CORE_C6_RESIDENCY */
+		0
+	};
+
+static bool msr_is_allowed(u32 reg, int cpu)
+{
+	u32 *q;
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+
+	if (!capable(CAP_COMPROMISE_KERNEL))
+		return 1;
+
+	for (q = msr_generic_wl;*q != 0; q++) {
+		if (*q == reg)
+			return 1;
+	}
+	if (c->x86_vendor == X86_VENDOR_INTEL) {
+		for (q = msr_intel_wl;*q != 0; q++) {
+			if (*q == reg)
+				return 1;
+		}
+	} else if (c->x86_vendor == X86_VENDOR_AMD) {
+		for (q = msr_amd_wl;*q != 0; q++) {
+			if (*q == reg)
+				return 1;
+		}
+	}
+	return 0;
+}
+
+#else /* X86_SECURE_BOOT */
+
+static bool msr_is_allowed(u32 reg)
+{
+	return 1;
+}
+
+#endif /* X86_SECURE_BOOT */
+#endif /* X86_SECURE_MSR_WHITELIST_H */

Re: [RFC] [PATCH] X86 MSR read whitelist

[Alan Cox]

> What exactly could go wrong with which MSR read?

Show me a pair of Intel and AMD architecture documents guaranteeing all
undefined, undocumented, and documented MSR reads for all processors have
no side effects.

> I just picked some MSRs I found in turbostat and cpupower.
> Len: It would be great if you could contribute to cpupower.
> It can do exactly the same than turbostat (but for all archs)
> and the fixups you send for turbostat are very easy to integrate
> into cpupower as well.

I suspect the number you need is very smal fortunately (and that they
could be properly exposed by a kernel interface that knows the CPU type
and what is needed for each - be that a chopped down msr driver or
something saner)

> > I would also look at MTRRs because mis-setting MTRRs
> > allows you to get firmware to do interesting things in certain
> > situations because the commands being issued to stuff like the GPU may
> > get corrupted.
> I am not aware of a mtrr interface to userspace.

/proc/mtrr


> +static u32 msr_intel_wl[] =
> +	{
> +		0x198, /* MSR_IA32_PERF_STATUS */
> +		0x1a0, /* MSR_IA32_MISC_ENABLES */
> +		0x1b0, /* MSR_IA32_ENERGY_PERF_BIAS */
> +		0xce,  /* MSR_NEHALEM_PLATFORM_INFO */
> +		0x1ad, /* MSR_NEHALEM_TURBO_RATIO_LIMIT */
> +		0x3f8, /* MSR_PKG_C3_RESIDENCY */
> +		0x3f9, /* MSR_PKG_C6_RESIDENCY */
> +		0x3fc, /* MSR_CORE_C3_RESIDENCY */
> +		0x3fd, /* MSR_CORE_C6_RESIDENCY */

I believe you need to verify precisely which processor each is
architecturally defined for and whitelist according to a CPU match.

Alan