backport of XSA-274 patch to 4.9.x kernel (could use a review)

backport of XSA-274 patch to 4.9.x kernel (could use a review)

[Chris Brannon]

I just got the following patch from a colleague.  It's a backport of
the XSA 274 kernel patch to 4.9.x kernels.  The kernel patch given in
the XSA would not apply cleanly.  Would someone mind reviewing it?  It
would be much appreciated.

commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.

This version applies to v4.9.

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.

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: 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)
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: backport of XSA-274 patch to 4.9.x kernel (could use a review)

[George Dunlap]

On Mon, Aug 6, 2018 at 8:10 PM, Chris Brannon <cmb@prgmr.com> wrote:
> I just got the following patch from a colleague.  It's a backport of
> the XSA 274 kernel patch to 4.9.x kernels.  The kernel patch given in
> the XSA would not apply cleanly.  Would someone mind reviewing it?  It
> would be much appreciated.
>
> commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.
>
> This version applies to v4.9.
>
> 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.
>
> 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: Sarah Newman <srn@prgmr.com>

I think you need to retain Andy's SoB, and add your own underneath.

This looks plausible to me -- Andy / Boris, any opinions?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: backport of XSA-274 patch to 4.9.x kernel (could use a review)

[Boris Ostrovsky]

On 08/07/2018 01:20 PM, George Dunlap wrote:
> On Mon, Aug 6, 2018 at 8:10 PM, Chris Brannon <cmb@prgmr.com> wrote:
>> I just got the following patch from a colleague.  It's a backport of
>> the XSA 274 kernel patch to 4.9.x kernels.  The kernel patch given in
>> the XSA would not apply cleanly.  Would someone mind reviewing it?  It
>> would be much appreciated.
>>
>> commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.
>>
>> This version applies to v4.9.
>>
>> 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.
>>
>> 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: Sarah Newman <srn@prgmr.com>
> I think you need to retain Andy's SoB, and add your own underneath.
>
> This looks plausible to me -- Andy / Boris, any opinions?


LGTM.

Note also that Andy's patch had slightly longer commit message
(including some of the tags that you are missing), with this suggestion:

      [ Note to stable maintainers: this should probably get applied to all
      kernels.  If you're nervous about that, a more conservative fix to
      add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
      also fix the problem. ]




-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: backport of XSA-274 patch to 4.9.x kernel (could use a review)

[Andy Lutomirski]

> On Aug 7, 2018, at 11:49 AM, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> 
>> On 08/07/2018 01:20 PM, George Dunlap wrote:
>>> On Mon, Aug 6, 2018 at 8:10 PM, Chris Brannon <cmb@prgmr.com> wrote:
>>> I just got the following patch from a colleague.  It's a backport of
>>> the XSA 274 kernel patch to 4.9.x kernels.  The kernel patch given in
>>> the XSA would not apply cleanly.  Would someone mind reviewing it?  It
>>> would be much appreciated.
>>> 
>>> commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.
>>> 
>>> This version applies to v4.9.
>>> 
>>> 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.
>>> 
>>> 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: Sarah Newman <srn@prgmr.com>
>> I think you need to retain Andy's SoB, and add your own underneath.
>> 
>> This looks plausible to me -- Andy / Boris, any opinions?
> 
> 
> LGTM.
> 
> Note also that Andy's patch had slightly longer commit message
> (including some of the tags that you are missing), with this suggestion:
> 
>       [ Note to stable maintainers: this should probably get applied to all
>       kernels.  If you're nervous about that, a more conservative fix to
>       add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
>       also fix the problem. ]
> 

On further review, I don’t like that suggestion. What if the callback came from user code. It’s not supposed to happen on modern kernels, but I’m not sure I trust that. 

> 
> 
> 
> -boris
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Sarah Newman]

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)
 
-- 
1.9.1

[PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Sarah Newman]

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)
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 4713 bytes --]

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)
>  

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 4713 bytes --]

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)
>  

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Sarah Newman]

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

Re: [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Sarah Newman]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Greg KH]

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

Re: [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Greg KH]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Andy Lutomirski]

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.

Re: [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Andy Lutomirski]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Andy Lutomirski]

On Mon, Jul 23, 2018 at 12:25 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sun, Jul 22, 2018 at 11:05:09AM -0700, Andy Lutomirski wrote:
>> 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.  If you're nervous about that, a more conservative fix to
>>  add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
>>  also fix the problem.]
>>
>> 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
>> 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>
>> ---
>>
>> I could also submit the conservative fix tagged for -stable and respin
>> this on top of it.  Ingo, Greg, what do you prefer?
>
> I don't care, this patch looks good to me to take as-is for the stable
> trees.  If you trust it in Linus's tree, it should be fine for others :)
>

My concern is more that something may work differently in older
kernels and there might be some subtle issue.  I'd be surprised, but
still.

Re: [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Andy Lutomirski]

On Mon, Jul 23, 2018 at 12:25 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sun, Jul 22, 2018 at 11:05:09AM -0700, Andy Lutomirski wrote:
>> 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.  If you're nervous about that, a more conservative fix to
>>  add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
>>  also fix the problem.]
>>
>> 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
>> 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>
>> ---
>>
>> I could also submit the conservative fix tagged for -stable and respin
>> this on top of it.  Ingo, Greg, what do you prefer?
>
> I don't care, this patch looks good to me to take as-is for the stable
> trees.  If you trust it in Linus's tree, it should be fine for others :)
>

My concern is more that something may work differently in older
kernels and there might be some subtle issue.  I'd be surprised, but
still.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Greg KH]

On Sun, Jul 22, 2018 at 11:05:09AM -0700, Andy Lutomirski wrote:
> 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.  If you're nervous about that, a more conservative fix to
>  add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
>  also fix the problem.]
> 
> 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
> 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>
> ---
> 
> I could also submit the conservative fix tagged for -stable and respin
> this on top of it.  Ingo, Greg, what do you prefer?

I don't care, this patch looks good to me to take as-is for the stable
trees.  If you trust it in Linus's tree, it should be fine for others :)

thanks,

greg k-h

Re: [PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Greg KH]

On Sun, Jul 22, 2018 at 11:05:09AM -0700, Andy Lutomirski wrote:
> 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.  If you're nervous about that, a more conservative fix to
>  add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
>  also fix the problem.]
> 
> 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
> 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>
> ---
> 
> I could also submit the conservative fix tagged for -stable and respin
> this on top of it.  Ingo, Greg, what do you prefer?

I don't care, this patch looks good to me to take as-is for the stable
trees.  If you trust it in Linus's tree, it should be fine for others :)

thanks,

greg k-h

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Andy Lutomirski]

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.  If you're nervous about that, a more conservative fix to
 add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
 also fix the problem.]

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
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>
---

I could also submit the conservative fix tagged for -stable and respin
this on top of it.  Ingo, Greg, what do you prefer?

 arch/x86/entry/entry_64.S | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 73a522d53b53..8ae7ffda8f98 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -981,7 +981,7 @@ ENTRY(\sym)
 
 	call	\do_sym
 
-	jmp	error_exit			/* %ebx: no swapgs flag */
+	jmp	error_exit
 	.endif
 END(\sym)
 .endm
@@ -1222,7 +1222,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)
 	UNWIND_HINT_FUNC
@@ -1269,7 +1268,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
@@ -1303,28 +1301,20 @@ 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)
 	UNWIND_HINT_REGS
 	DISABLE_INTERRUPTS(CLBR_ANY)
 	TRACE_IRQS_OFF
-	testl	%ebx, %ebx
-	jnz	retint_kernel
+	testb	$3, CS(%rsp)
+	jz	retint_kernel
 	jmp	retint_user
 END(error_exit)
 
-- 
2.17.1

[PATCH] x86/entry/64: Remove %ebx handling from error_entry/exit

[Andy Lutomirski]

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.  If you're nervous about that, a more conservative fix to
 add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
 also fix the problem.]

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
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>
---

I could also submit the conservative fix tagged for -stable and respin
this on top of it.  Ingo, Greg, what do you prefer?

 arch/x86/entry/entry_64.S | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 73a522d53b53..8ae7ffda8f98 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -981,7 +981,7 @@ ENTRY(\sym)
 
 	call	\do_sym
 
-	jmp	error_exit			/* %ebx: no swapgs flag */
+	jmp	error_exit
 	.endif
 END(\sym)
 .endm
@@ -1222,7 +1222,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)
 	UNWIND_HINT_FUNC
@@ -1269,7 +1268,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
@@ -1303,28 +1301,20 @@ 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)
 	UNWIND_HINT_REGS
 	DISABLE_INTERRUPTS(CLBR_ANY)
 	TRACE_IRQS_OFF
-	testl	%ebx, %ebx
-	jnz	retint_kernel
+	testb	$3, CS(%rsp)
+	jz	retint_kernel
 	jmp	retint_user
 END(error_exit)
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel