Re: [PATCH V4 01/50] x86/entry: Add fence for kernel entry swapgs in paranoid_entry()

[Lai Jiangshan]

On 2021/10/27 20:19, Borislav Petkov wrote:
> On Tue, Oct 26, 2021 at 10:13:31PM +0800, Lai Jiangshan wrote:
> > From: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
> >
> > 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.
> >
> > Fixes: Commit 96b2371413e8f ("x86/entry/64: Switch CR3 before SWAPGS in paranoid entry")
> > Cc: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > Cc: Chang S. Bae <chang.seok.bae@xxxxxxxxx>
> > Cc: Sasha Levin <sashal@xxxxxxxxxx>
> > Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
> > ---
> >   arch/x86/entry/entry_64.S | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> > index e38a4cf795d9..1f98188e83ef 100644
> > --- a/arch/x86/entry/entry_64.S
> > +++ b/arch/x86/entry/entry_64.S
> > @@ -898,6 +898,7 @@ SYM_CODE_START_LOCAL(paranoid_entry)
> >   	rdmsr
> >   	testl	%edx, %edx
> >   	jns	.Lparanoid_entry_swapgs
> > +	FENCE_SWAPGS_KERNEL_ENTRY
>
> Wouldn't it be cleaner to simply move the FENCE up from under the swapgs
> to before the jns instruction?

Sure, I will move it just before the testl instruction.

> This way you kill speculation for both cases and have a single fence.