Re: [PATCH 3/6] x86/entry: Use PUSH_AND_CLEAR_REGS for compat

[Josh Poimboeuf]

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@xxxxxxxxxx> 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