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

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 <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dominik Brodowski <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Fixes 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface")

Reported-and-tested-by "M. Vefa Bicakci" <[email protected]>

Signed-off-by Sarah Newman <[email protected]>

Quoted 38 lines · 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.

Quoted 15 lines

Cc Brian Gerst <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dominik Brodowski <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Fixes 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface")

Reported-and-tested-by "M. Vefa Bicakci" <[email protected]>

Signed-off-by Sarah Newman <[email protected]>

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

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

Quoted 56 lines · George Dunlap

Quoted 39 lines · 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.

Quoted 15 lines

Cc Brian Gerst <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dominik Brodowski <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Fixes 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface")

Reported-and-tested-by "M. Vefa Bicakci" <[email protected]>

Signed-off-by Sarah Newman <[email protected]>

Quoted 3 lines

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

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 <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dominik Brodowski <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Fixes 3ac6d8c787b8 ("x86/entry/64: Clear registers for

exceptions/interrupts, to reduce speculation attack surface")

Reported-and-tested-by "M. Vefa Bicakci" <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Signed-off-by Sarah Newman <[email protected]>

---
 arch/x86/entry/entry_64.S | 19 ++++---------------

 1 file changed, 4 insertions(+), 15 deletions(-)

arch/x86/entry/entry_64.S +0 -0

index d58d8dc..0dab47a 100644
@@ -774,7 +774,7 @@ ENTRY(\sym)

  call \do_sym

arch/x86/entry/entry_64.S +0 -1

-	jmp	error_exit			/* %ebx: no

swapgs flag */

arch/x86/entry/entry_64.S +1 -0

+	jmp	error_exit

  .endif
 END(\sym)
 .endm

arch/x86/entry/entry_64.S +0 -0

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

arch/x86/entry/entry_64.S +0 -0

@@ -1087,7 +1086,6 @@ ENTRY(error_entry)
  	 * for these here too.
  	 */

 .Lerror_kernelspace:

arch/x86/entry/entry_64.S +0 -1

-	incl	%ebx

  leaq native_irq_return_iret(%rip), %rcx
  cmpq %rcx, RIP+8(%rsp)
  je .Lerror_bad_iret

arch/x86/entry/entry_64.S +0 -0

@@ -1119,28 +1117,19 @@ ENTRY(error_entry)

  /*
   * Pretend that the exception came from user mode: set up pt_regs

arch/x86/entry/entry_64.S +0 -1

-	 * as if we faulted immediately after IRET and clear EBX so

that

arch/x86/entry/entry_64.S +1 -1

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

arch/x86/entry/entry_64.S +0 -1

-	decl	%ebx

  jmp .Lerror_entry_from_usermode_after_swapgs
 END(error_entry)

arch/x86/entry/entry_64.S +0 -5

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

arch/x86/entry/entry_64.S +0 -1

-	movl	%ebx, %eax

  DISABLE_INTERRUPTS(CLBR_NONE)
  TRACE_IRQS_OFF

arch/x86/entry/entry_64.S +2 -2

-	testl	%eax, %eax
-	jnz	retint_kernel
+	testb	$3, CS(%rsp)
+	jz	retint_kernel

  jmp retint_user
 END(error_exit)

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 <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dominik Brodowski <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Fixes 3ac6d8c787b8 ("x86/entry/64: Clear registers for

exceptions/interrupts, to reduce speculation attack surface")

Reported-and-tested-by "M. Vefa Bicakci" <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Signed-off-by Sarah Newman <[email protected]>

---
 arch/x86/entry/entry_64.S | 19 ++++---------------

 1 file changed, 4 insertions(+), 15 deletions(-)

arch/x86/entry/entry_64.S +0 -0

index d58d8dc..0dab47a 100644
@@ -774,7 +774,7 @@ ENTRY(\sym)

  call \do_sym

arch/x86/entry/entry_64.S +0 -1

-	jmp	error_exit			/* %ebx: no

swapgs flag */

arch/x86/entry/entry_64.S +1 -0

+	jmp	error_exit

  .endif
 END(\sym)
 .endm

arch/x86/entry/entry_64.S +0 -0

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

arch/x86/entry/entry_64.S +0 -0

@@ -1087,7 +1086,6 @@ ENTRY(error_entry)
  	 * for these here too.
  	 */

 .Lerror_kernelspace:

arch/x86/entry/entry_64.S +0 -1

-	incl	%ebx

  leaq native_irq_return_iret(%rip), %rcx
  cmpq %rcx, RIP+8(%rsp)
  je .Lerror_bad_iret

arch/x86/entry/entry_64.S +0 -0

@@ -1119,28 +1117,19 @@ ENTRY(error_entry)

  /*
   * Pretend that the exception came from user mode: set up pt_regs

arch/x86/entry/entry_64.S +0 -1

-	 * as if we faulted immediately after IRET and clear EBX so

that

arch/x86/entry/entry_64.S +1 -1

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

arch/x86/entry/entry_64.S +0 -1

-	decl	%ebx

  jmp .Lerror_entry_from_usermode_after_swapgs
 END(error_entry)

arch/x86/entry/entry_64.S +0 -5

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

arch/x86/entry/entry_64.S +0 -1

-	movl	%ebx, %eax

  DISABLE_INTERRUPTS(CLBR_NONE)
  TRACE_IRQS_OFF

arch/x86/entry/entry_64.S +2 -2

-	testl	%eax, %eax
-	jnz	retint_kernel
+	testb	$3, CS(%rsp)
+	jz	retint_kernel

  jmp retint_user
 END(error_exit)

Quoted 3 lines · Sarah Newman

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.

Quoted 129 lines

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 <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dominik Brodowski <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Fixes 3ac6d8c787b8 ("x86/entry/64: Clear registers for

exceptions/interrupts, to reduce speculation attack surface")

Reported-and-tested-by "M. Vefa Bicakci" <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Signed-off-by Sarah Newman <[email protected]>

---
 arch/x86/entry/entry_64.S | 19 ++++---------------

 1 file changed, 4 insertions(+), 15 deletions(-)

arch/x86/entry/entry_64.S +0 -0

index d58d8dc..0dab47a 100644
@@ -774,7 +774,7 @@ ENTRY(\sym)

  call \do_sym

arch/x86/entry/entry_64.S +0 -1

-	jmp	error_exit			/* %ebx: no

swapgs flag */

arch/x86/entry/entry_64.S +1 -0

+	jmp	error_exit

  .endif
 END(\sym)
 .endm

arch/x86/entry/entry_64.S +0 -0

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

arch/x86/entry/entry_64.S +0 -0

@@ -1087,7 +1086,6 @@ ENTRY(error_entry)
  	 * for these here too.
  	 */

 .Lerror_kernelspace:

arch/x86/entry/entry_64.S +0 -1

-	incl	%ebx

  leaq native_irq_return_iret(%rip), %rcx
  cmpq %rcx, RIP+8(%rsp)
  je .Lerror_bad_iret

arch/x86/entry/entry_64.S +0 -0

@@ -1119,28 +1117,19 @@ ENTRY(error_entry)

  /*
   * Pretend that the exception came from user mode: set up pt_regs

arch/x86/entry/entry_64.S +0 -1

-	 * as if we faulted immediately after IRET and clear EBX so

that

arch/x86/entry/entry_64.S +1 -1

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

arch/x86/entry/entry_64.S +0 -1

-	decl	%ebx

  jmp .Lerror_entry_from_usermode_after_swapgs
 END(error_entry)

arch/x86/entry/entry_64.S +0 -5

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

arch/x86/entry/entry_64.S +0 -1

-	movl	%ebx, %eax

  DISABLE_INTERRUPTS(CLBR_NONE)
  TRACE_IRQS_OFF

arch/x86/entry/entry_64.S +2 -2

-	testl	%eax, %eax
-	jnz	retint_kernel
+	testb	$3, CS(%rsp)
+	jz	retint_kernel

  jmp retint_user
 END(error_exit)

Quoted 4 lines

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Quoted 3 lines · Sarah Newman

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.

Quoted 133 lines

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 <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dominik Brodowski <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Fixes 3ac6d8c787b8 ("x86/entry/64: Clear registers for

exceptions/interrupts, to reduce speculation attack surface")

Reported-and-tested-by "M. Vefa Bicakci" <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Signed-off-by Sarah Newman <[email protected]>

---
 arch/x86/entry/entry_64.S | 19 ++++---------------

 1 file changed, 4 insertions(+), 15 deletions(-)

arch/x86/entry/entry_64.S +0 -0

index d58d8dc..0dab47a 100644
@@ -774,7 +774,7 @@ ENTRY(\sym)

  call \do_sym

arch/x86/entry/entry_64.S +0 -1

-	jmp	error_exit			/* %ebx: no

swapgs flag */

arch/x86/entry/entry_64.S +1 -0

+	jmp	error_exit

  .endif
 END(\sym)
 .endm

arch/x86/entry/entry_64.S +0 -0

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

arch/x86/entry/entry_64.S +0 -0

@@ -1087,7 +1086,6 @@ ENTRY(error_entry)
  	 * for these here too.
  	 */

 .Lerror_kernelspace:

arch/x86/entry/entry_64.S +0 -1

-	incl	%ebx

  leaq native_irq_return_iret(%rip), %rcx
  cmpq %rcx, RIP+8(%rsp)
  je .Lerror_bad_iret

arch/x86/entry/entry_64.S +0 -0

@@ -1119,28 +1117,19 @@ ENTRY(error_entry)

  /*
   * Pretend that the exception came from user mode: set up pt_regs

arch/x86/entry/entry_64.S +0 -1

-	 * as if we faulted immediately after IRET and clear EBX so

that

arch/x86/entry/entry_64.S +1 -1

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

arch/x86/entry/entry_64.S +0 -1

-	decl	%ebx

  jmp .Lerror_entry_from_usermode_after_swapgs
 END(error_entry)

arch/x86/entry/entry_64.S +0 -5

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

arch/x86/entry/entry_64.S +0 -1

-	movl	%ebx, %eax

  DISABLE_INTERRUPTS(CLBR_NONE)
  TRACE_IRQS_OFF

arch/x86/entry/entry_64.S +2 -2

-	testl	%eax, %eax
-	jnz	retint_kernel
+	testb	$3, CS(%rsp)
+	jz	retint_kernel

  jmp retint_user
 END(error_exit)

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Quoted 8 lines · David Woodhouse

Quoted 3 lines · Sarah Newman

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?

Quoted 8 lines · David Woodhouse

Quoted 3 lines · Sarah Newman

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?

Quoted 15 lines · Sarah Newman

Quoted 8 lines · David Woodhouse

Quoted 3 lines · Sarah Newman

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.

Quoted 4 lines

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}

Quoted 15 lines · Sarah Newman

Quoted 8 lines · David Woodhouse

Quoted 3 lines · Sarah Newman

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.

Quoted 4 lines

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}