linux-kernel - Re: [PATCH 3/6] x86/entry: Use PUSH_AND_CLEAR

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] [day] [month] [year] [list]

Message-ID: <20220520155533.qke6e3m2tl5lk6xo@treble>
Date:   Fri, 20 May 2022 08:55:33 -0700
From:   Josh Poimboeuf <jpoimboe@...nel.org>
To:     Lai Jiangshan <jiangshanlai@...il.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Guenter Roeck <linux@...ck-us.net>, X86 ML <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Brian Gerst <brgerst@...il.com>,
        Andrew Cooper <Andrew.Cooper3@...rix.com>,
        Mark Rutland <mark.rutland@....com>,
        Borislav Petkov <bp@...e.de>
Subject: Re: [PATCH 3/6] x86/entry: Use PUSH_AND_CLEAR_REGS for compat

On Fri, May 20, 2022 at 09:11:55AM +0800, Lai Jiangshan wrote:
> On Fri, May 20, 2022 at 1:35 AM Josh Poimboeuf <jpoimboe@...nel.org> wrote:
> >
> > diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
> > index ed2be3615b50..f76e674d22c4 100644
> > --- a/arch/x86/entry/entry_64_compat.S
> > +++ b/arch/x86/entry/entry_64_compat.S
> > @@ -200,7 +200,7 @@ SYM_INNER_LABEL(entry_SYSCALL_compat_safe_stack, SYM_L_GLOBAL)
> >  SYM_INNER_LABEL(entry_SYSCALL_compat_after_hwframe, SYM_L_GLOBAL)
> >         movl    %eax, %eax              /* discard orig_ax high bits */
> >         pushq   %rax                    /* pt_regs->orig_ax */
> > -       PUSH_AND_CLEAR_REGS rax=$-ENOSYS
> > +       PUSH_AND_CLEAR_REGS rcx=%rbp rax=$-ENOSYS
> 
> Some comments need to be here to explain why %rcx is stashed in %rbp.
> 
> The code doing the stash in userspace may be in
> arch/x86/entry/vdso/vdso32/system_call.S (see SYSCALL_SEQUENCE)

I do agree a comment would be good, but looking at that maze, I'm not
sure I'm qualified to give it a proper one ;-)

My best theory is: __kernel_vsyscall() stashes CX in BP before SYSCALL
can overwrite it, because SYSCALL uses CX to stash the return address.
And then PUSH_AND_CLEAR_REGS puts the original CX value back in pt_regs,
because CX is (presumably?) a syscall function argument.

My patch description said that CX must have gotten corrupted in user
space, but that's wrong because __kernel_vsyscall() pushes/pops CX
around the SYSCALL.

But alas it's too late to fix the commit log because it's already been
committed and the tip maintainers are getting pull requests ready for
the merge window.

-- 
Josh