commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.
This version applies to v4.9.
From Andy Lutomirski, original author:
error_entry and error_exit communicate the user vs kernel status of the frame using %ebx. This is unnecessary -- the information is in regs->cs. Just use regs->cs.
This makes error_entry simpler and makes error_exit more robust.
It also fixes a nasty bug. Before all the Spectre nonsense, The xen_failsafe_callback entry point returned like this:
ALLOC_PT_GPREGS_ON_STACK SAVE_C_REGS SAVE_EXTRA_REGS ENCODE_FRAME_POINTER jmp error_exit
And it did not go through error_entry. This was bogus: RBX contained garbage, and error_exit expected a flag in RBX. Fortunately, it generally contained *nonzero* garbage, so the correct code path was used. As part of the Spectre fixes, code was added to clear RBX to mitigate certain speculation attacks. Now, depending on kernel configuration, RBX got zeroed and, when running some Wine workloads, the kernel crashes. This was introduced by:
commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface")
With this patch applied, RBX is no longer needed as a flag, and the problem goes away.
I suspect that malicious userspace could use this bug to crash the kernel even without the offending patch applied, though.
[Historical note: I wrote this patch as a cleanup before I was aware of the bug it fixed.]
[Note to stable maintainers: this should probably get applied to all kernels.]
Cc: Brian Gerst brgerst@gmail.com Cc: Borislav Petkov bp@alien8.de Cc: Dominik Brodowski linux@dominikbrodowski.net Cc: Ingo Molnar mingo@redhat.com Cc: "H. Peter Anvin" hpa@zytor.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Boris Ostrovsky boris.ostrovsky@oracle.com Cc: Juergen Gross jgross@suse.com Cc: xen-devel@lists.xenproject.org Cc: x86@kernel.org Cc: stable@vger.kernel.org Cc: Andy Lutomirski luto@kernel.org Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") Reported-and-tested-by: "M. Vefa Bicakci" m.v.b@runbox.com Signed-off-by: Andy Lutomirski luto@kernel.org Signed-off-by: Sarah Newman srn@prgmr.com --- arch/x86/entry/entry_64.S | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index d58d8dc..0dab47a 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -774,7 +774,7 @@ ENTRY(\sym)
call \do_sym
- jmp error_exit /* %ebx: no swapgs flag */ + jmp error_exit .endif END(\sym) .endm @@ -1043,7 +1043,6 @@ END(paranoid_exit)
/* * Save all registers in pt_regs, and switch gs if needed. - * Return: EBX=0: came from user mode; EBX=1: otherwise */ ENTRY(error_entry) cld @@ -1087,7 +1086,6 @@ ENTRY(error_entry) * for these here too. */ .Lerror_kernelspace: - incl %ebx leaq native_irq_return_iret(%rip), %rcx cmpq %rcx, RIP+8(%rsp) je .Lerror_bad_iret @@ -1119,28 +1117,19 @@ ENTRY(error_entry)
/* * Pretend that the exception came from user mode: set up pt_regs - * as if we faulted immediately after IRET and clear EBX so that - * error_exit knows that we will be returning to user mode. + * as if we faulted immediately after IRET. */ mov %rsp, %rdi call fixup_bad_iret mov %rax, %rsp - decl %ebx jmp .Lerror_entry_from_usermode_after_swapgs END(error_entry)
- -/* - * On entry, EBX is a "return to kernel mode" flag: - * 1: already in kernel mode, don't need SWAPGS - * 0: user gsbase is loaded, we need SWAPGS and standard preparation for return to usermode - */ ENTRY(error_exit) - movl %ebx, %eax DISABLE_INTERRUPTS(CLBR_NONE) TRACE_IRQS_OFF - testl %eax, %eax - jnz retint_kernel + testb $3, CS(%rsp) + jz retint_kernel jmp retint_user END(error_exit)
On Wed, 2018-08-08 at 10:35 -0700, Sarah Newman wrote:
commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.
This version applies to v4.9.
I think you can kill the 'xorl %ebx,%ebx' from error_entry too but yes, this does want to go to 4.9 and earlier because the 'Fixes:' tag is a bit of a lie — the problem existed before that, at least in theory.
From Andy Lutomirski, original author:
error_entry and error_exit communicate the user vs kernel status of the frame using %ebx. This is unnecessary -- the information is in regs->cs. Just use regs->cs.
This makes error_entry simpler and makes error_exit more robust.
It also fixes a nasty bug. Before all the Spectre nonsense, The xen_failsafe_callback entry point returned like this:
ALLOC_PT_GPREGS_ON_STACK SAVE_C_REGS SAVE_EXTRA_REGS ENCODE_FRAME_POINTER jmp error_exit
And it did not go through error_entry. This was bogus: RBX contained garbage, and error_exit expected a flag in RBX. Fortunately, it generally contained *nonzero* garbage, so the correct code path was used. As part of the Spectre fixes, code was added to clear RBX to mitigate certain speculation attacks. Now, depending on kernel configuration, RBX got zeroed and, when running some Wine workloads, the kernel crashes. This was introduced by:
commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface")
With this patch applied, RBX is no longer needed as a flag, and the problem goes away.
I suspect that malicious userspace could use this bug to crash the kernel even without the offending patch applied, though.
[Historical note: I wrote this patch as a cleanup before I was aware of the bug it fixed.]
[Note to stable maintainers: this should probably get applied to all kernels.]
Cc: Brian Gerst brgerst@gmail.com Cc: Borislav Petkov bp@alien8.de Cc: Dominik Brodowski linux@dominikbrodowski.net Cc: Ingo Molnar mingo@redhat.com Cc: "H. Peter Anvin" hpa@zytor.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Boris Ostrovsky boris.ostrovsky@oracle.com Cc: Juergen Gross jgross@suse.com Cc: xen-devel@lists.xenproject.org Cc: x86@kernel.org Cc: stable@vger.kernel.org Cc: Andy Lutomirski luto@kernel.org Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") Reported-and-tested-by: "M. Vefa Bicakci" m.v.b@runbox.com Signed-off-by: Andy Lutomirski luto@kernel.org Signed-off-by: Sarah Newman srn@prgmr.com
arch/x86/entry/entry_64.S | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index d58d8dc..0dab47a 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -774,7 +774,7 @@ ENTRY(\sym) call \do_sym
- jmp error_exit /* %ebx: no
swapgs flag */
- jmp error_exit
.endif END(\sym) .endm @@ -1043,7 +1043,6 @@ END(paranoid_exit) /* * Save all registers in pt_regs, and switch gs if needed.
- Return: EBX=0: came from user mode; EBX=1: otherwise
*/ ENTRY(error_entry) cld @@ -1087,7 +1086,6 @@ ENTRY(error_entry) * for these here too. */ .Lerror_kernelspace:
- incl %ebx
leaq native_irq_return_iret(%rip), %rcx cmpq %rcx, RIP+8(%rsp) je .Lerror_bad_iret @@ -1119,28 +1117,19 @@ ENTRY(error_entry) /* * Pretend that the exception came from user mode: set up pt_regs
- * as if we faulted immediately after IRET and clear EBX so
that
- * error_exit knows that we will be returning to user mode.
- * as if we faulted immediately after IRET.
*/ mov %rsp, %rdi call fixup_bad_iret mov %rax, %rsp
- decl %ebx
jmp .Lerror_entry_from_usermode_after_swapgs END(error_entry)
-/*
- On entry, EBX is a "return to kernel mode" flag:
- * 1: already in kernel mode, don't need SWAPGS
- * 0: user gsbase is loaded, we need SWAPGS and standard
preparation for return to usermode
- */
ENTRY(error_exit)
- movl %ebx, %eax
DISABLE_INTERRUPTS(CLBR_NONE) TRACE_IRQS_OFF
- testl %eax, %eax
- jnz retint_kernel
- testb $3, CS(%rsp)
- jz retint_kernel
jmp retint_user END(error_exit)
On 08/09/2018 05:41 AM, David Woodhouse wrote:
On Wed, 2018-08-08 at 10:35 -0700, Sarah Newman wrote:
commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.
This version applies to v4.9.
I think you can kill the 'xorl %ebx,%ebx' from error_entry too but yes, this does want to go to 4.9 and earlier because the 'Fixes:' tag is a bit of a lie — the problem existed before that, at least in theory.
The commit 2140a9942 "x86/entry/64: Relax pvops stub clobber specifications" was what removed the "movl %ebx, %eax" line later on originally, but it was the commit 3ac6d8c787b8 that removed the 'xorl %ebx,%ebx'. So these weren't matched.
I don't know if it's a concern, but if someone had gone to the effort of backporting the original commit 3ac6d8c787b83, adding the removal of 'xorl %ebx,%ebx' to this patch would create merge conflicts. For that reason and given the line is harmless, should it be left in?
From Andy Lutomirski, original author:
error_entry and error_exit communicate the user vs kernel status of the frame using %ebx. This is unnecessary -- the information is in regs->cs. Just use regs->cs.
This makes error_entry simpler and makes error_exit more robust.
It also fixes a nasty bug. Before all the Spectre nonsense, The xen_failsafe_callback entry point returned like this:
ALLOC_PT_GPREGS_ON_STACK SAVE_C_REGS SAVE_EXTRA_REGS ENCODE_FRAME_POINTER jmp error_exit
And it did not go through error_entry. This was bogus: RBX contained garbage, and error_exit expected a flag in RBX. Fortunately, it generally contained *nonzero* garbage, so the correct code path was used. As part of the Spectre fixes, code was added to clear RBX to mitigate certain speculation attacks. Now, depending on kernel configuration, RBX got zeroed and, when running some Wine workloads, the kernel crashes. This was introduced by:
commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface")
With this patch applied, RBX is no longer needed as a flag, and the problem goes away.
I suspect that malicious userspace could use this bug to crash the kernel even without the offending patch applied, though.
[Historical note: I wrote this patch as a cleanup before I was aware of the bug it fixed.]
[Note to stable maintainers: this should probably get applied to all kernels.]
Cc: Brian Gerst brgerst@gmail.com Cc: Borislav Petkov bp@alien8.de Cc: Dominik Brodowski linux@dominikbrodowski.net Cc: Ingo Molnar mingo@redhat.com Cc: "H. Peter Anvin" hpa@zytor.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Boris Ostrovsky boris.ostrovsky@oracle.com Cc: Juergen Gross jgross@suse.com Cc: xen-devel@lists.xenproject.org Cc: x86@kernel.org Cc: stable@vger.kernel.org Cc: Andy Lutomirski luto@kernel.org Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface") Reported-and-tested-by: "M. Vefa Bicakci" m.v.b@runbox.com Signed-off-by: Andy Lutomirski luto@kernel.org Signed-off-by: Sarah Newman srn@prgmr.com
arch/x86/entry/entry_64.S | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index d58d8dc..0dab47a 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -774,7 +774,7 @@ ENTRY(\sym) call \do_sym
- jmp error_exit /* %ebx: no
swapgs flag */
- jmp error_exit
.endif END(\sym) .endm @@ -1043,7 +1043,6 @@ END(paranoid_exit) /* * Save all registers in pt_regs, and switch gs if needed.
- Return: EBX=0: came from user mode; EBX=1: otherwise
*/ ENTRY(error_entry) cld @@ -1087,7 +1086,6 @@ ENTRY(error_entry) * for these here too. */ .Lerror_kernelspace:
- incl %ebx
leaq native_irq_return_iret(%rip), %rcx cmpq %rcx, RIP+8(%rsp) je .Lerror_bad_iret @@ -1119,28 +1117,19 @@ ENTRY(error_entry) /* * Pretend that the exception came from user mode: set up pt_regs
- * as if we faulted immediately after IRET and clear EBX so
that
- * error_exit knows that we will be returning to user mode.
- * as if we faulted immediately after IRET.
*/ mov %rsp, %rdi call fixup_bad_iret mov %rax, %rsp
- decl %ebx
jmp .Lerror_entry_from_usermode_after_swapgs END(error_entry)
-/*
- On entry, EBX is a "return to kernel mode" flag:
- * 1: already in kernel mode, don't need SWAPGS
- * 0: user gsbase is loaded, we need SWAPGS and standard
preparation for return to usermode
- */
ENTRY(error_exit)
- movl %ebx, %eax
DISABLE_INTERRUPTS(CLBR_NONE) TRACE_IRQS_OFF
- testl %eax, %eax
- jnz retint_kernel
- testb $3, CS(%rsp)
- jz retint_kernel
jmp retint_user END(error_exit)
Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, Aug 10, 2018 at 12:23:45AM -0700, Sarah Newman wrote:
On 08/09/2018 05:41 AM, David Woodhouse wrote:
On Wed, 2018-08-08 at 10:35 -0700, Sarah Newman wrote:
commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.
This version applies to v4.9.
I think you can kill the 'xorl %ebx,%ebx' from error_entry too but yes, this does want to go to 4.9 and earlier because the 'Fixes:' tag is a bit of a lie — the problem existed before that, at least in theory.
The commit 2140a9942 "x86/entry/64: Relax pvops stub clobber specifications" was what removed the "movl %ebx, %eax" line later on originally, but it was the commit 3ac6d8c787b8 that removed the 'xorl %ebx,%ebx'. So these weren't matched.
I don't know if it's a concern, but if someone had gone to the effort of backporting the original commit 3ac6d8c787b83, adding the removal of 'xorl %ebx,%ebx' to this patch would create merge conflicts. For that reason and given the line is harmless, should it be left in?
I need some kind of agreement here for me to know what to do with this patch... {hint}
thanks,
greg k-h
On Thu, Aug 16, 2018 at 8:19 AM, Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Aug 10, 2018 at 12:23:45AM -0700, Sarah Newman wrote:
On 08/09/2018 05:41 AM, David Woodhouse wrote:
On Wed, 2018-08-08 at 10:35 -0700, Sarah Newman wrote:
commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.
This version applies to v4.9.
I think you can kill the 'xorl %ebx,%ebx' from error_entry too but yes, this does want to go to 4.9 and earlier because the 'Fixes:' tag is a bit of a lie — the problem existed before that, at least in theory.
The commit 2140a9942 "x86/entry/64: Relax pvops stub clobber specifications" was what removed the "movl %ebx, %eax" line later on originally, but it was the commit 3ac6d8c787b8 that removed the 'xorl %ebx,%ebx'. So these weren't matched.
I don't know if it's a concern, but if someone had gone to the effort of backporting the original commit 3ac6d8c787b83, adding the removal of 'xorl %ebx,%ebx' to this patch would create merge conflicts. For that reason and given the line is harmless, should it be left in?
I need some kind of agreement here for me to know what to do with this patch... {hint}
I would remove the xorl.
If there's an actual candidate patch, I'd be happy to read it.
linux-stable-mirror@lists.linaro.org
-
Andy Lutomirski -
David Woodhouse -
Greg KH -
Sarah Newman