Backtrace after invalid XRSTOR after "x86/fault: BUG() when uaccess helpers fault on kernel addresses"

Backtrace after invalid XRSTOR after "x86/fault: BUG() when uaccess helpers fault on kernel addresses"

[Sebastian Andrzej Siewior]

Commit 75045f77f7a7 ("x86/extable: Introduce _ASM_EXTABLE_UA for uaccess
fixups") made copy_user_to_xregs() -> XSTATE_OP() use _ASM_EXTABLE_UA.
Commit 9da3f2b74054 ("x86/fault: BUG() when uaccess helpers fault on
kernel addresses") then decided that a #GP is not good and has to be
reported loudly.

I had a TC which sets a few invalid bits in xstate which is used by
copy_user_to_xregs() on sig-return. Before that change I had:
| sig-xstate-bum[2253] bad frame in rt_sigreturn frame:0000000056078134 ip:7f9da336c227 sp:7ffc871325e8 orax:ffffffffffffffff in  libc-2.27.so[7f9da3358000+146000]

after those two patches are applied:
|BUG: GPF in non-whitelisted uaccess (non-canonical address?)
|general protection fault: 0000 [#1] PREEMPT SMP NOPTI
|CPU: 26 PID: 2236 Comm: sig-xstate-bum Not tainted 4.20.0-rc3 #45
|RIP: 0010:__fpu__restore_sig+0x1c1/0x540
|Code: 02 00 00 48 8b 95 58 ff ff ff 48 f7 d2 48 21 d0 0f 85 6e 03 00 00 0f 01 cb 48 8b 85 58 ff ff ff 48 89 df 48 89 c2 48 c1 ea 20 <48> 0f ae 2f 31 db 0f 01 ca 85 db 0f 84 d7 00 00 00 4c 89 f7 bb ff
|Call Trace:
| fpu__restore_sig+0x28/0x40
| restore_sigcontext+0x13a/0x180
| __ia32_sys_rt_sigreturn+0xae/0x100
| do_syscall_64+0x4f/0x100
| entry_SYSCALL_64_after_hwframe+0x44/0xa9
|RIP: 0033:0x7f9b06aea227
|---[ end trace a45ac23b593e9ab0 ]---

The expected behaviour would that `xrstor' performs a #GP and this does
not a produce a backtrace like that and copy_user_to_fxregs() returns an
error.
copy_user_to_fxregs() / user_insn() does not have this behaviour and
that also might generate a #GP (if invalid bits are set in MCSR).
What do we do?

Sebastian

Re: Backtrace after invalid XRSTOR after "x86/fault: BUG() when uaccess helpers fault on kernel addresses"

[Jann Horn]

On Mon, Nov 26, 2018 at 5:59 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> Commit 75045f77f7a7 ("x86/extable: Introduce _ASM_EXTABLE_UA for uaccess
> fixups") made copy_user_to_xregs() -> XSTATE_OP() use _ASM_EXTABLE_UA.
> Commit 9da3f2b74054 ("x86/fault: BUG() when uaccess helpers fault on
> kernel addresses") then decided that a #GP is not good and has to be
> reported loudly.
>
> I had a TC which sets a few invalid bits in xstate which is used by
> copy_user_to_xregs() on sig-return. Before that change I had:
> | sig-xstate-bum[2253] bad frame in rt_sigreturn frame:0000000056078134 ip:7f9da336c227 sp:7ffc871325e8 orax:ffffffffffffffff in  libc-2.27.so[7f9da3358000+146000]
>
> after those two patches are applied:
> |BUG: GPF in non-whitelisted uaccess (non-canonical address?)
> |general protection fault: 0000 [#1] PREEMPT SMP NOPTI
> |CPU: 26 PID: 2236 Comm: sig-xstate-bum Not tainted 4.20.0-rc3 #45
> |RIP: 0010:__fpu__restore_sig+0x1c1/0x540
> |Code: 02 00 00 48 8b 95 58 ff ff ff 48 f7 d2 48 21 d0 0f 85 6e 03 00 00 0f 01 cb 48 8b 85 58 ff ff ff 48 89 df 48 89 c2 48 c1 ea 20 <48> 0f ae 2f 31 db 0f 01 ca 85 db 0f 84 d7 00 00 00 4c 89 f7 bb ff
> |Call Trace:
> | fpu__restore_sig+0x28/0x40
> | restore_sigcontext+0x13a/0x180
> | __ia32_sys_rt_sigreturn+0xae/0x100
> | do_syscall_64+0x4f/0x100
> | entry_SYSCALL_64_after_hwframe+0x44/0xa9
> |RIP: 0033:0x7f9b06aea227
> |---[ end trace a45ac23b593e9ab0 ]---
>
> The expected behaviour would that `xrstor' performs a #GP and this does
> not a produce a backtrace like that and copy_user_to_fxregs() returns an
> error.
> copy_user_to_fxregs() / user_insn() does not have this behaviour and
> that also might generate a #GP (if invalid bits are set in MCSR).
> What do we do?

Bleh. This code has to use normal _ASM_EXTABLE. _ASM_EXTABLE_UA is
(almost, with the exception of stuff like probe_kernel_read() and
exact_copy_from_user()) only for code that isn't expected to throw
things other than #PF with a userspace address. I must have missed
this when looking at the documentation for XRSTOR, or something like
that...

[PATCH v2] x86/fpu: XRSTOR is expected to raise #GP

[Jann Horn]

commit 75045f77f7a7 ("x86/extable: Introduce _ASM_EXTABLE_UA for uaccess
fixups") incorrectly replaced the fixup entry for XSTATE_OP with a
user-#PF-only fixup. XRSTOR can also raise #GP if the xstate content is
invalid, and _ASM_EXTABLE_UA doesn't expect that.
Change this fixup back to _ASM_EXTABLE so that #GP gets fixed up.

Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Fixes: 75045f77f7a7 ("x86/extable: Introduce _ASM_EXTABLE_UA for uaccess fixups")
Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/x86/include/asm/fpu/internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 5f7290e6e954..69dcdf195b61 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -226,7 +226,7 @@ static inline void copy_fxregs_to_kernel(struct fpu *fpu)
 		     "3: movl $-2,%[err]\n\t"				\
 		     "jmp 2b\n\t"					\
 		     ".popsection\n\t"					\
-		     _ASM_EXTABLE_UA(1b, 3b)				\
+		     _ASM_EXTABLE(1b, 3b)				\
 		     : [err] "=r" (err)					\
 		     : "D" (st), "m" (*st), "a" (lmask), "d" (hmask)	\
 		     : "memory")
-- 
2.20.0.rc0.387.gc7a69e6b6c-goog

[tip:x86/urgent] x86/fpu: Use the correct exception table macro in the XSTATE_OP wrapper

[tip-bot for Jann Horn]

Commit-ID:  ac26d1f74cfc19c8dc9d533b5f20e99dbee3d9bd
Gitweb:     https://git.kernel.org/tip/ac26d1f74cfc19c8dc9d533b5f20e99dbee3d9bd
Author:     Jann Horn <jannh@google.com>
AuthorDate: Tue, 27 Nov 2018 14:32:00 +0100
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Tue, 27 Nov 2018 17:55:45 +0100

x86/fpu: Use the correct exception table macro in the XSTATE_OP wrapper

Commit

  75045f77f7a7 ("x86/extable: Introduce _ASM_EXTABLE_UA for uaccess fixups")

incorrectly replaced the fixup entry for XSTATE_OP with a user-#PF-only
fixup. XRSTOR can also raise #GP if the xstate content is invalid,
and _ASM_EXTABLE_UA doesn't expect that. Change this fixup back to
_ASM_EXTABLE so that #GP gets fixed up.

Fixes: 75045f77f7a7 ("x86/extable: Introduce _ASM_EXTABLE_UA for uaccess fixups")
Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: kernel-hardening@lists.openwall.com
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20181126165957.xhsyu2dhyy45mrjo@linutronix.de
Link: https://lkml.kernel.org/r/20181127133200.38322-1-jannh@google.com
---
 arch/x86/include/asm/fpu/internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 5f7290e6e954..69dcdf195b61 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -226,7 +226,7 @@ static inline void copy_fxregs_to_kernel(struct fpu *fpu)
 		     "3: movl $-2,%[err]\n\t"				\
 		     "jmp 2b\n\t"					\
 		     ".popsection\n\t"					\
-		     _ASM_EXTABLE_UA(1b, 3b)				\
+		     _ASM_EXTABLE(1b, 3b)				\
 		     : [err] "=r" (err)					\
 		     : "D" (st), "m" (*st), "a" (lmask), "d" (hmask)	\
 		     : "memory")