UNTRAIN_RET in native_irq_return_ldt

UNTRAIN_RET in native_irq_return_ldt

[Alexandre Chartre]

Hi,

I think there is an issue in native_irq_return_ldt: UNTRAIN_RET is used and can
clobber %rax which is expected to be the user rax.

A simple fix would be to preserve %rax:

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index a4ba162e52c3..f1fe05289d84 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -728,7 +728,11 @@ native_irq_return_ldt:
         pushq   %rdi                            /* Stash user RDI */
         swapgs                                  /* to kernel GS */
         SWITCH_TO_KERNEL_CR3 scratch_reg=%rdi   /* to kernel CR3 */
+
+       /* UNTRAIN_RET can clobber %rax, so preserve it */
+       movq    %rax, %rdi
         UNTRAIN_RET
+       movq    %rdi, %rax
  
         movq    PER_CPU_VAR(espfix_waddr), %rdi
         movq    %rax, (0*8)(%rdi)               /* user RAX */


But I wonder if we really need to use UNTRAIN_RET in native_irq_return_ldt because
I think we reach this point from the kernel after untrain has already be done,
and it looks like we don't do ret afterward (the code just fixup the stack and
then iret).

Thanks,

alex.

Re: UNTRAIN_RET in native_irq_return_ldt

[Peter Zijlstra]

On Tue, Jul 12, 2022 at 08:20:44PM +0200, Alexandre Chartre wrote:
> 
> Hi,
> 
> I think there is an issue in native_irq_return_ldt: UNTRAIN_RET is used and can
> clobber %rax which is expected to be the user rax.

and cx and dx..

> A simple fix would be to preserve %rax:
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index a4ba162e52c3..f1fe05289d84 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -728,7 +728,11 @@ native_irq_return_ldt:
>         pushq   %rdi                            /* Stash user RDI */
>         swapgs                                  /* to kernel GS */
>         SWITCH_TO_KERNEL_CR3 scratch_reg=%rdi   /* to kernel CR3 */
> +
> +       /* UNTRAIN_RET can clobber %rax, so preserve it */
> +       movq    %rax, %rdi
>         UNTRAIN_RET
> +       movq    %rdi, %rax
>         movq    PER_CPU_VAR(espfix_waddr), %rdi
>         movq    %rax, (0*8)(%rdi)               /* user RAX */
> 

Which still leaves cx and dx scrambled.

> But I wonder if we really need to use UNTRAIN_RET in native_irq_return_ldt because
> I think we reach this point from the kernel after untrain has already be done,
> and it looks like we don't do ret afterward (the code just fixup the stack and
> then iret).

Yes, I think removing it is fine, the objtool unret validation also
doesn't complain about it not being there (I really should have written
that validation before doing these patches, not after, oh well).

[tip: x86/urgent] x86/entry: Remove UNTRAIN_RET from native_irq_return_ldt

[tip-bot2 for Alexandre Chartre]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     d16e0b26672066035439b2f49887f6576c4a3689
Gitweb:        https://git.kernel.org/tip/d16e0b26672066035439b2f49887f6576c4a3689
Author:        Alexandre Chartre <alexandre.chartre@oracle.com>
AuthorDate:    Wed, 13 Jul 2022 21:58:08 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Thu, 14 Jul 2022 09:45:12 +02:00

x86/entry: Remove UNTRAIN_RET from native_irq_return_ldt

UNTRAIN_RET is not needed in native_irq_return_ldt because RET
untraining has already been done at this point.

In addition, when the RETBleed mitigation is IBPB, UNTRAIN_RET clobbers
several registers (AX, CX, DX) so here it trashes user values which are
in these registers.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lore.kernel.org/r/35b0d50f-12d1-10c3-f5e8-d6c140486d4a@oracle.com
---
 arch/x86/entry/entry_64.S | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 285e043..9953d96 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -727,7 +727,6 @@ native_irq_return_ldt:
 	pushq	%rdi				/* Stash user RDI */
 	swapgs					/* to kernel GS */
 	SWITCH_TO_KERNEL_CR3 scratch_reg=%rdi	/* to kernel CR3 */
-	UNTRAIN_RET
 
 	movq	PER_CPU_VAR(espfix_waddr), %rdi
 	movq	%rax, (0*8)(%rdi)		/* user RAX */