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.
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).
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 <[email protected]>
AuthorDate: Wed, 13 Jul 2022 21:58:08 +02:00
Committer: Borislav Petkov <[email protected]>
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 <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 */