[12/27] x86/msr: Restrict MSR access when the kernel is locked down

Commit Message

Matthew Garrett March 25, 2019, 10:09 p.m. UTC

From: Matthew Garrett <[e-mail]>

Writing to MSRs should not be allowed if the kernel is locked down, since
it could lead to execution of arbitrary code in kernel mode.  Based on a
patch by Kees Cook.

MSR accesses are logged for the purposes of building up a whitelist as per
Alan Cox's suggestion.

Signed-off-by: Matthew Garrett <[e-mail]>
Signed-off-by: David Howells <[e-mail]>
Acked-by: Kees Cook <[e-mail]>
Reviewed-by: Thomas Gleixner <[e-mail]>
cc: [e-mail]
Signed-off-by: Matthew Garrett <[e-mail]>
---
 arch/x86/kernel/msr.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Thomas Gleixner March 25, 2019, 11:40 p.m. UTC | #1

Matthew,

On Mon, 25 Mar 2019, Matthew Garrett wrote:

> From: Matthew Garrett <[e-mail]>
> 
> Writing to MSRs should not be allowed if the kernel is locked down, since
> it could lead to execution of arbitrary code in kernel mode.  Based on a
> patch by Kees Cook.
> 
> MSR accesses are logged for the purposes of building up a whitelist as per
> Alan Cox's suggestion.
> 
> Signed-off-by: Matthew Garrett <[e-mail]>
> Signed-off-by: David Howells <[e-mail]>
> Acked-by: Kees Cook <[e-mail]>
> Reviewed-by: Thomas Gleixner <[e-mail]>

I'm pretty sure, that I reviewed a different version of this, but due to
the lack of:

 1) A version indicator in the subject line, i.e. [PATCH v7 12/27]

 2) A simple change indicator after the --- separator, e.g.

    v6 -> v7: Add MRS logging to dmesg .....

It's tedious to figure out what actually changed here. I just know for sure
that the printk wasn't there before.

It's not a huge effort adding such information, but it's very helpful for
those who are supposed to look at your patches. Those people are drowned in
patches so making it as easy as it goes would be very appreciated.

> +++ b/arch/x86/kernel/msr.c
> @@ -84,6 +84,11 @@ static ssize_t msr_write(struct file *file, const char __user *buf,
>  	int err = 0;
>  	ssize_t bytes = 0;
>  
> +	if (kernel_is_locked_down("Direct MSR access")) {
> +		pr_info("Direct access to MSR %x\n", reg);

I'm really not fond of this at all. /dev/msr should simply die.

Maintaining a whitelist for this is a horrible idea as you will get a
gazillion of excuses why access to a particular MSR is sane. And I'm
neither interested in these discussions nor interested in adding the
whitelist to this trainwreck,

The right thing to do is to provide sane interfaces and that's where we are
moving to. So simply blocking the access with locked down mode might be
very helpful to accelerate that.

Thanks,

	tglx

Patch

diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index 4588414e2561..f5a2cf07972f 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -84,6 +84,11 @@  static ssize_t msr_write(struct file *file, const char __user *buf,
 	int err = 0;
 	ssize_t bytes = 0;
 
+	if (kernel_is_locked_down("Direct MSR access")) {
+		pr_info("Direct access to MSR %x\n", reg);
+		return -EPERM;
+	}
+
 	if (count % 8)
 		return -EINVAL;	/* Invalid chunk size */
 
@@ -135,6 +140,11 @@  static long msr_ioctl(struct file *file, unsigned int ioc, unsigned long arg)
 			err = -EFAULT;
 			break;
 		}
+		if (kernel_is_locked_down("Direct MSR access")) {
+			pr_info("Direct access to MSR %x\n", regs[1]); /* Display %ecx */
+			err = -EPERM;
+			break;
+		}
 		err = wrmsr_safe_regs_on_cpu(cpu, regs);
 		if (err)
 			break;