linux-kernel - Re: UNTRAIN_RET in native_irq_return

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 13 Jul 2022 11:50:50 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Alexandre Chartre <alexandre.chartre@...cle.com>
Cc:     Borislav Petkov <bp@...en8.de>, X86 ML <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: UNTRAIN_RET in native_irq_return_ldt

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