* UNTRAIN_RET in native_irq_return_ldt
@ 2022-07-12 18:20 Alexandre Chartre
2022-07-13 9:50 ` Peter Zijlstra
2022-07-14 11:08 ` [tip: x86/urgent] x86/entry: Remove UNTRAIN_RET from native_irq_return_ldt tip-bot2 for Alexandre Chartre
0 siblings, 2 replies; 3+ messages in thread
From: Alexandre Chartre @ 2022-07-12 18:20 UTC (permalink / raw)
To: Peter Zijlstra, Borislav Petkov; +Cc: X86 ML, LKML
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.
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: UNTRAIN_RET in native_irq_return_ldt
2022-07-12 18:20 UNTRAIN_RET in native_irq_return_ldt Alexandre Chartre
@ 2022-07-13 9:50 ` Peter Zijlstra
2022-07-14 11:08 ` [tip: x86/urgent] x86/entry: Remove UNTRAIN_RET from native_irq_return_ldt tip-bot2 for Alexandre Chartre
1 sibling, 0 replies; 3+ messages in thread
From: Peter Zijlstra @ 2022-07-13 9:50 UTC (permalink / raw)
To: Alexandre Chartre; +Cc: Borislav Petkov, X86 ML, LKML
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).
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tip: x86/urgent] x86/entry: Remove UNTRAIN_RET from native_irq_return_ldt
2022-07-12 18:20 UNTRAIN_RET in native_irq_return_ldt Alexandre Chartre
2022-07-13 9:50 ` Peter Zijlstra
@ 2022-07-14 11:08 ` tip-bot2 for Alexandre Chartre
1 sibling, 0 replies; 3+ messages in thread
From: tip-bot2 for Alexandre Chartre @ 2022-07-14 11:08 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Alexandre Chartre, Borislav Petkov, x86, linux-kernel
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 */
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-14 11:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-12 18:20 UNTRAIN_RET in native_irq_return_ldt Alexandre Chartre
2022-07-13 9:50 ` Peter Zijlstra
2022-07-14 11:08 ` [tip: x86/urgent] x86/entry: Remove UNTRAIN_RET from native_irq_return_ldt tip-bot2 for Alexandre Chartre
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.