linux-kernel - Re: [PATCH] x86/entry/64: Fix CR3 restore order in paranoid

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: <20180214222757.zda2xvytzb7jjfyy@gmail.com>
Date:   Wed, 14 Feb 2018 23:27:57 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Dave Hansen <dave.hansen@...el.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH] x86/entry/64: Fix CR3 restore order in paranoid_exit()


* Josh Poimboeuf <jpoimboe@...hat.com> wrote:

> Dave was right, my patch was obviously bogus.  I couldn't figure out a
> real reproducer, so I made an artificial one (see below) and can confirm
> that your patch fixes it.
> 
> I would resubmit the patch, but now you're the author, so I'm not sure
> how that works with the SOB.

I'd just have made you the author of the oneliner fix, in fair payment for writing 
the changelog! ;-)

For anything larger and more complex we can do:

  Signed-off-by: Ingo Molnar <mingo@...nel.org>
  Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
  Signed-off-by: Ingo Molnar <mingo@...nel.org>

Which is technically an unusual but still correct 'SoB chain' describing the 
true route of the patch.

See for example these scheduler commits:

  8c0944cee7af: sched/deadline: Rename __dl_clear() to __dl_sub()
  07f06cb3b5f6: sched: Start stopper early
  7675104990ed: sched: Implement lockless wake-queues

In particular 7675104990ed is a good example:

  Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
  [tweaks, adjustments, comments, etc.]
  Signed-off-by: Davidlohr Bueso <dbueso@...e.de>
  Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
  Acked-by: Thomas Gleixner <tglx@...utronix.de>
  Signed-off-by: Ingo Molnar <mingo@...nel.org>

With his developer hat on PeterZ sent out a first implementation which Davidlohr 
fixed, tested and signed off on - which PeterZ then applied with his maintainer 
hat on, which Thomas acked and I committed to the scheduler git tree.

It would require two commits (one of which could easily be a bisection-breaker) to 
express this in any other way.

But such chains are pretty rare and very much the exception: usually either the 
original author takes care of everything, or the secondary author makes so many 
changes that the secondary author becomes the primary author and the original 
author is credited via a copyright notice and/or a tag like:

  Originally-From: Ingo Molnar <mingo@...nel.org>

> Feel free to add my
> 
>   Reported-and-tested-by: Josh Poimboeuf <jpoimboe@...hat.com>

Thanks and added!

BTW:

> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -325,6 +325,8 @@ syscall_return_via_sysret:
>  	 */
>  	SWITCH_TO_USER_CR3_STACK scratch_reg=%rdi
>  
> +	int3
> +
>  	popq	%rdi
>  	popq	%rsp
>  	USERGS_SYSRET64
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 3d9b2308e7fa..74fabcdf6c36 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -605,6 +605,8 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
>  		goto exit;
>  #endif
>  
> +	goto exit;

That's a very clever way to test such races!

Thanks,

	Ingo