Date:   Thu, 18 Nov 2021 16:54:33 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Lai Jiangshan <jiangshanlai@gmail.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        Lai Jiangshan <laijs@linux.alibaba.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        "Chang S . Bae" <chang.seok.bae@intel.com>,
        Sasha Levin <sashal@kernel.org>,
        Andy Lutomirski <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH V5 01/50] x86/entry: Add fence for kernel entry swapgs in
 paranoid_entry()
Message-ID: <YZZ3OUaMjMIhvj70@hirez.programming.kicks-ass.net>
References: <20211110115736.3776-1-jiangshanlai@gmail.com>
 <20211110115736.3776-2-jiangshanlai@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211110115736.3776-2-jiangshanlai@gmail.com>
Precedence: bulk

On Wed, Nov 10, 2021 at 07:56:47PM +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> Commit 18ec54fdd6d18 ("x86/speculation: Prepare entry code for Spectre
> v1 swapgs mitigations") adds FENCE_SWAPGS_{KERNEL|USER}_ENTRY
> for conditional swapgs.  And in paranoid_entry(), it uses only
> FENCE_SWAPGS_KERNEL_ENTRY for both branches.  It is because the fence
> is required for both cases since the CR3 write is conditinal even PTI
> is enabled.
> 
> But commit 96b2371413e8f ("x86/entry/64: Switch CR3 before SWAPGS in
> paranoid entry") switches the code order and changes the branches.
> And it misses the needed FENCE_SWAPGS_KERNEL_ENTRY for user gsbase case.
> 
> Add it back by moving FENCE_SWAPGS_KERNEL_ENTRY up to cover both branches.
> 
> Fixes: Commit 96b2371413e8f ("x86/entry/64: Switch CR3 before SWAPGS in paranoid entry")
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Chang S. Bae <chang.seok.bae@intel.com>
> Cc: Sasha Levin <sashal@kernel.org>
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
>  arch/x86/entry/entry_64.S | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index e38a4cf795d9..14ffe12807ba 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -888,6 +888,13 @@ SYM_CODE_START_LOCAL(paranoid_entry)
>  	ret
>  
>  .Lparanoid_entry_checkgs:
> +	/*
> +	 * The above SAVE_AND_SWITCH_TO_KERNEL_CR3 macro doesn't do an
> +	 * unconditional CR3 write, even in the PTI case.  So do an lfence
> +	 * to prevent GS speculation, regardless of whether PTI is enabled.
> +	 */
> +	FENCE_SWAPGS_KERNEL_ENTRY
> +
>  	/* EBX = 1 -> kernel GSBASE active, no restore required */
>  	movl	$1, %ebx
>  	/*
> @@ -903,13 +910,6 @@ SYM_CODE_START_LOCAL(paranoid_entry)
>  .Lparanoid_entry_swapgs:
>  	swapgs
>  
> -	/*
> -	 * The above SAVE_AND_SWITCH_TO_KERNEL_CR3 macro doesn't do an
> -	 * unconditional CR3 write, even in the PTI case.  So do an lfence
> -	 * to prevent GS speculation, regardless of whether PTI is enabled.
> -	 */
> -	FENCE_SWAPGS_KERNEL_ENTRY
> -
>  	/* EBX = 0 -> SWAPGS required on exit */
>  	xorl	%ebx, %ebx
>  	ret

I'm confused, shouldn't the LFENCE be between SWAPGS and future uses of
GS prefix?

In the old code, before 96b2371413e8f, we had:

	swapgs
	SAVE_AND_SWITCH_TO_KERNEL_CR3
	FENCE_SWAPGS_KERNEL_ENTRY

	// %gs user comes here..

And the comment made sense, since if SAVE_AND_SWITCH_TO_KERNEL_CR3 would
imply an unconditional CR3 write, the LFENCE would not be needed.

Then along gomes 96b2371413e8f and changes the order to:

	SAVE_AND_SWITCH_TO_KERNEL_CR3
	swapgs
	FENCE_SWAPGS_KERNEL_ENTRY
	// %gs user comes here..

But now the comment is crazy talk, because even if the CR3 write were
unconditional, it'd be pointless, since it's not after SWAPGS, but we
still have the LFENCE in the right place.

But now you want to make it:

	SAVE_AND_SWITCH_TO_KERNEL_CR3
	FENCE_SWAPGS_KERNEL_ENTRY
	swapgs
	// %gs user comes here..

And there's nothing left and speculation can use the old %gs for our
user and things go sideways. Hmm?


(on a completely unrelated note, I find KERNEL_ENTRY and USER_ENTRY
utterly confusing)