linux-kernel - Re: [PATCH V4 1/7] x86/traps: Move pt_regs only in fixup_bad

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]

Message-ID: <CAJhGHyBFbtyUs-nf0+gWm2a3hiS5BxZ3jk=sbGNw-4ShB8AtzQ@mail.gmail.com>
Date:   Thu, 7 Apr 2022 15:03:08 +0800
From:   Lai Jiangshan <jiangshanlai@...il.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>, X86 ML <x86@...nel.org>,
        Lai Jiangshan <jiangshan.ljs@...group.com>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Tai <thomas.tai@...cle.com>,
        "Chang S. Bae" <chang.seok.bae@...el.com>,
        Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH V4 1/7] x86/traps: Move pt_regs only in fixup_bad_iret()

On Thu, Apr 7, 2022 at 3:00 AM Borislav Petkov <bp@...en8.de> wrote:
>
> On Fri, Mar 18, 2022 at 10:30:10PM +0800, Lai Jiangshan wrote:
> > From: Lai Jiangshan <jiangshan.ljs@...group.com>
> >
> > fixup_bad_iret() and sync_regs() have similar arguments and do similar
> > work that copies full or partial pt_regs to a place and switches stack
> > after return.  They are quite the same, but fixup_bad_iret() not only
> > copies the pt_regs but also the return address of error_entry() while
>
> What return address of error_entry()? You lost me here.

"return address" is the return address of a function which is
error_entry() here.

https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html

Or error_entry_ret in struct bad_iret_stack which is being removed
in the change.

>
> So fixup_bad_iret() gets the stack ptr passed in by doing:
>
>         mov     %rsp, %rdi
>         call    fixup_bad_iret
>         mov     %rax, %rsp
>
>
> and error_regs()
>
>         movq    %rsp, %rdi                      /* arg0 = pt_regs pointer */
>         call    sync_regs
>         movq    %rax, %rsp                      /* switch stack */
>
> the same way.

They are not the same way.

sync_regs() is called before the return address of error_entry()
popped into %r12 while fixup_bad_iret() is called with the return
address of error_entry() still on the stack.  And the primitives of
fixup_bad_iret() and sync_regs() are different which also means
they are not the same way.

After this change, they become the same way.

IMO, sync_regs() is grace while fixup_bad_iret() is a bad C function
or is not a pure C function because it is handling the return address
of its parent function which is better done by the compiler or ASM
code.

>
> Confused.
>
> > It is prepared for later patch to do the stack switch after the
> > error_entry() which simplifies the code further.
>
> Looking at your next patch, is all this dance done just so that you can
> do
>
>         leaq    8(%rsp), %rdi
>
> in order to pass in pt_regs to both functions?
>
> And get rid of the saving/restoring %r12?
>
> Is that what the whole noise is about?

The point is to make fixup_bad_iret() a normal C function and
the same as sync_regs() in calling.

The next patch makes error_entry() as a bunch of ASM code compiled
from a C function and pave the road to really convert it to a C
function.

Getting rid of the saving/restoring the return address in %r12
is necessary since a C function can't save/restore the return
address.

Thanks
Lai

>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

I'm sorry for using top-posting and "This patch".  I remember it.