x86/cpu: Fix MSR value truncation issue

From: Borislav Petkov <[email protected]>

So sparse rightfully complains that the u64 MSR value we're writing into the STAR MSR, i.e. 0xc0000081, is being truncated:

./arch/x86/include/asm/msr.h:193:36: warning: cast truncates bits from constant value (23001000000000 becomes 0)

because the actual value doesn't fit into the unsigned 32-bit quantity which are the @low and @high wrmsrl() parameters.

This is not a problem, practically, because gcc is actually being smart enough here and does the right thing:

.loc 3 87 0

  xorl    %esi, %esi		# we needz a 32-bit zero
  movl    $2293776, %edx	# 0x00230010 == (__USER32_CS << 16) | __KERNEL_CS go into the high bits
  movl    $-1073741695, %ecx	# MSR_STAR, i.e., 0xc0000081
  movl    %esi, %eax		# low order 32 bits in the MSR which are 0

#APP
# 87 "./arch/x86/include/asm/msr.h" 1
wrmsr

More specifically, MSR_STAR[31:0] is being set to 0. That field is reserved on Intel and on AMD it is 32-bit SYSCALL Target EIP.

I'd strongly guess because Intel doesn't have SYSCALL in compat/legacy mode and we're using SYSENTER and INT80 there. And for compat syscalls in long mode we use CSTAR.

So let's fix the sparse warning by writing SYSRET and SYSCALL CS and SS into the high 32-bit half of STAR and 0 in the low half explicitly.

[ Actually, if we had to be precise, we would have to read what's in
STAR[31:0] and write it back unchanged on Intel and write 0 on AMD. I guess the current writing to 0 is still ok since Intel can apparently stomach it. ]

The resulting code is identical to what we have above:

.loc 3 87 0

  xorl    %esi, %esi      # tmp104
  movl    $2293776, %eax  #, tmp103
  movl    $-1073741695, %ecx      #, tmp102
  movl    %esi, %edx      # tmp104, tmp104

...

wrmsr

---
 arch/x86/kernel/cpu/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4ddd780aeac9..42cfb0b8249c 100644
@@ -1186,7 +1186,7 @@ void syscall_init(void)
 	 * They both write to the same internal register. STAR allows to
 	 * set CS/DS but only a 32bit target. LSTAR sets the 64bit rip.
 	 */
-	wrmsrl(MSR_STAR,  ((u64)__USER32_CS)<<48  | ((u64)__KERNEL_CS)<<32);
+	wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
 	wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
 
 #ifdef CONFIG_IA32_EMULATION

2.3.5

Is this with or without:

commit 47edb65178cb7056c2eea0b6c41a7d8c84547192

Author: Andy Lutomirski <[email protected]>
Date:   Thu Jul 23 12:14:40 2015 -0700

x86/asm/msr: Make wrmsrl() a function

If that patch is applied, then I think that gcc is just being dumb and that we should consider tweaking wrmsrl to avoid generating the warning. Maybe change (u32)val to (u32)(val & 0xffffffffull)?

I don't see why we should uglify the caller when the problem is some combination of gcc and the wrmsrl implementation.

--Andy

with.

I'm playing ontop of 4.3-rc7.

Huh, why?

gcc is actually being smart by completely avoiding the shift to the upper bits and puts those directly into %edx and sparse simply says that we're truncating some bits.

I actually think that sparse is correct in saying that some bits are going to be zeroed out even though we want that here.

That works.

Why uglify?

We're basically making explicit that we write the high 32-bits with the SYSCALL and SYSRET CS and SS and we set the low 32-bit explicitly to 0:

	wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);

versus setting the low 32-bits to zero *implicitly*:

	wrmsrl(MSR_STAR,  ((u64)__USER32_CS)<<48  | ((u64)__KERNEL_CS)<<32);

due to that shifting to the left filling up the 32-bit with zeroes.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Want to add that to the patch or make it another patch?

Fair enough. I suppose that this thing is a handful of separate fields as opposed to being just a number. I'm fine with making both changes.

--Andy

Yeah, I'll make another one as it is going to document why we're explicitly ANDing with 0xffffffffull.

You mean MSR_STAR?

Just the two upper 16-bit values. The lower 32-bit are reserved on Intel while on AMD they're "32-bit SYSCALL Target EIP", meaning that you can use SYSCALL on 32-bit too.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Yeah, exactly. When I wrote the "make wrmsrl a function" patch, I was dealing with MSRs that were genuinely 64-bit numbers, except that in some cases they provably fit in 32 bits, and gcc was unhappy.

--Andy

---

From: Borislav Petkov <[email protected]>
Date: Sat, 31 Oct 2015 12:40:48 +0100
Subject: [PATCH] x86/MSR: Chop off lower 32-bit value

sparse complains that the cast truncates the high bits. But here we really do know what we're doing and we need the lower 32 bits only as the @low argument. So make that explicit.

---
 arch/x86/include/asm/msr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 77d8b284e4a7..86133827c75c 100644
@@ -190,7 +190,7 @@ static inline void wrmsr(unsigned msr, unsigned low, unsigned high)
 
 static inline void wrmsrl(unsigned msr, u64 val)
 {
-	native_write_msr(msr, (u32)val, (u32)(val >> 32));
+	native_write_msr(msr, (u32)(val & 0xffffffffULL), (u32)(val >> 32));
 }
 
 /* wrmsr with exception handling */

2.3.5

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

So I was wondering what would happen if I used SYSCALL on 32-bit AMD.

This is what happens on a normal system:

$ strace -f ./syscall
execve("./syscall", ["./syscall"], [/* 24 vars */]) = 0
--- SIGILL {si_signo=SIGILL, si_code=ILL_ILLOPN, si_addr=0x80480e8} ---
+++ killed by SIGILL +++
Illegal instruction

Wondering who causes the SIGILL and after some code staring, it is MSR EFER.SCE which we don't enable on 32-bit.

And, because I like to cause fire (woahahahah... /me rubs hands and laughs ominously), I went and toggled that bit.

Oh well, we bomb out, as expected:

 BUG: sleeping function called from invalid context at /mnt/kernel/kernel/linux-2.6/arch/x86/mm/fault.c:1191
 in_atomic(): 0, irqs_disabled(): 1, pid: 2567, name: syscall
 1 lock held by syscall/2567:
  #0:  (&mm->mmap_sem){++++++}, at: [<c10447f7>] __do_page_fault+0xf7/0x3f0
 irq event stamp: 1812
 hardirqs last  enabled at (1811): [<c165f29a>] restore_all_notrace+0x0/0xe
 hardirqs last disabled at (1812): [<c1660145>] error_code+0x31/0x3c
 softirqs last  enabled at (988): [<c1059e5b>] __do_softirq+0x37b/0x440
 softirqs last disabled at (965): [<c1005749>] do_softirq_own_stack+0x39/0x50
 CPU: 1 PID: 2567 Comm: syscall Not tainted 4.3.0+ #1
 Hardware name: LENOVO 30515QG/30515QG, BIOS 8RET30WW (1.12 ) 09/15/2011
  00000000 00000000 bff53b20 c12fdfa2 00000000 bff53b48 c107a9bc c181aca4
  00000000 00000001 00000a07 f2cb3830 f2cb3500 00000000 00000000 bff53b7c
  c107aae6 f453f70c 00000001 bff53bd0 00000000 bff53b7c c109ee4d 00000001
 Call Trace:
 kernel tried to execute NX-protected page - exploit attempt? (uid: 0)
 BUG: unable to handle kernel NULL pointer dereference at   (null)
 IP: [<  (null)>]   (null)
 *pdpt = 0000000032e0b001 *pde = 0000000000000000 
 Oops: 0010 [#1] PREEMPT SMP 
 Modules linked in: ipv6 usbhid kvm_amd rtsx_pci_sdmmc kvm mmc_core snd_hda_codec_conexant snd_hda_codec_generic snd_hda_codec_hdmi pcspkr snd_hda_intel k10temp ohci_pci snd_hda_codec snd_hwdep snd_hda_core snd_pcm rtsx_pci mfd_core ohci_hcd battery snd_timer radeon thinkpad_acpi nvram ehci_pci ehci_hcd snd soundcore video ac button thermal
 CPU: 1 PID: 2567 Comm: syscall Not tainted 4.3.0+ #1
 Hardware name: LENOVO 30515QG/30515QG, BIOS 8RET30WW (1.12 ) 09/15/2011
 task: f2cb3500 ti: f2d74000 task.ti: f2d74000
 EIP: 0000:[<00000000>] EFLAGS: 00010086 CPU: 1
 EIP is at 0x0
 EAX: 00000000 EBX: 00000000 ECX: 080480ea EDX: 00000000
 ESI: 00000000 EDI: 00000000 EBP: bff53c1c ESP: bff53c0c
  DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0008
 CR0: 8005003b CR2: 00000000 CR3: 33af5900 CR4: 000006f0
 Stack:
  00000000 00000000 00000000 00000000 00000000 00000001 bff54df4 00000000
  bff54dfe bff54e0c bff54e18 bff54e31 bff54e3c bff54e4c bff54e6e bff54e81
  bff54e94 bff54e9e bff54eb2 bff54efe bff54f07 bff54f18 bff54f20 bff54f2b
 Call Trace:
 Code:  Bad EIP value.
 EIP: [<00000000>] 0x0 SS:ESP 0008:bff53c0c
 CR2: 0000000000000000
 ---[ end trace fa036c454007a131 ]---
 PANIC: double fault, gdt at f7bb7000 [255 bytes]
 double fault, tss at f7bbe9c0
 eip = c104afc3, esp = bff539dc
 eax = 00000000, ebx = f453f680, ecx = ffffffff, edx = f453f680
 esi = ffffffff, edi = f453f680

Nice.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Not terribly surprising :) Someone (I forget who) told me that 32-bit SYSCALL (native 32-bit, not compat) was so full of errata that it was unusable. Even without errata, I don't really see how it would work well -- there's no MSR_SYSCALL_MASK, so we can't mask off TF when SYSCALL happens, and I don't see how we're expected to handle SYSCALL with TF set on a 32-bit kernel unless we route #DB through a task gate, which I'm reasonably confident no one wants to do.

--Andy

No, showstopper appears much earlier: it is only supported on AMD. Which would mean, yet another vendor special-handling. And I don't think it's worth it.

Yeah, yeah, it might still be faster than SYSENTER, but 32-bit?! Srsly?! I'm surprised that thing still builds even. :-)

Of course there is:

MSRC000_0084 SYSCALL Flag Mask (SYSCALL_FLAG_MASK):

31:0 - Mask: SYSCALL flag mask. Read-write. Reset: 0000_0000h. This register holds the EFLAGS mask used by the SYSCALL instruction. 1=Clear the corresponding EFLAGS bit when executing the SYSCALL instruction.

Intel has that too, except again, no SYSCALL in legacy mode on Intel.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

I had tried to implement it when the K6 came out, but the major problem was that implementation set an internal flag that forced return to userspace with SYSRET. IRET would fault, which made task switching a big problem.

Specifically, the SYSCALL description for the K6 has this text:
"The CS and SS registers should not be modified by the operating system between the
execution of the SYSCALL instruction and its corresponding SYSRET instruction."

It's likely that behavior has been fixed on modern 64-bit AMD cpus running in legacy mode, but I haven't tested it. It's not really worth pursuing.

SYSCALL_FLAG_MASK was added with the 64-bit processors. It's not used in legacy mode according to the AMD docs.

--
Brian Gerst