Re: [PATCH v2.1] x86/retpoline: Fill return stack buffer on vmexit

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Thu, Jan 11, 2018 at 11:37:18AM +0000, David Woodhouse wrote:
> In accordance with the Intel and AMD documentation, we need to overwrite
> all entries in the RSB on exiting a guest, to prevent malicious branch
> target predictions from affecting the host kernel. This is needed both
> for retpoline and for IBRS.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> v2: Reduce the size of the ALTERNATIVE insns, fix .align (again)!
>     Sent in private email for testing, hence this second public post is
> v2.1: Add CONFIG_RETPOLINE around RSB stuffing
> 
> Sorry, Josh, objtool broke again! :)
> 
>  arch/x86/include/asm/nospec-branch.h | 76 ++++++++++++++++++++++++++++++++++++
>  arch/x86/kvm/svm.c                   |  4 ++
>  arch/x86/kvm/vmx.c                   |  4 ++
>  3 files changed, 84 insertions(+)
> 
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index 7d70ea9..b55ff79 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -7,6 +7,50 @@
>  #include <asm/alternative-asm.h>
>  #include <asm/cpufeatures.h>
>  
> +/*
> + * Fill the CPU return stack buffer.
> + *
> + * Each entry in the RSB, if used for a speculative 'ret', contains an
> + * infinite 'pause; jmp' loop to capture speculative execution.
> + *
> + * This is required in various cases for retpoline and IBRS-based
> + * mitigations for the Spectre variant 2 vulnerability. Sometimes to
> + * eliminate potentially bogus entries from the RSB, and sometimes
> + * purely to ensure that it doesn't get empty, which on some CPUs would
> + * allow predictions from other (unwanted!) sources to be used.
> + *
> + * We define a CPP macro such that it can be used from both .S files and
> + * inline assembly. It's possible to do a .macro and then include that
> + * from C via asm(".include <asm/nospec-branch.h>") but let's not go there.
> + */
> +
> +#define RSB_CLEAR_LOOPS		32	/* To forcibly overwrite all entries */
> +#define RSB_FILL_LOOPS		16	/* To avoid underflow */
> +
> +#define __FILL_RETURN_BUFFER_PREP(reg, nr)	\
> +	mov	$(nr/2), reg
> +
> +#define __FILL_RETURN_BUFFER(reg, nr, sp, uniq)	\
> +	.align	16;				\
> +.Ldo_call1_ ## uniq:				\
> +	call	.Ldo_call2_ ## uniq;		\
> +.Ltrap1_ ## uniq:				\
> +	pause;					\
> +	jmp	.Ltrap1_ ## uniq;		\
> +	.align	16;				\
> +.Ldo_call2_ ## uniq:				\
> +	call	.Ldo_loop_ ## uniq;		\
> +.Ltrap2_ ## uniq:				\
> +	pause;					\
> +	jmp	.Ltrap2_ ## uniq;		\
> +	.align	16;				\
> +.Ldo_loop_ ## uniq:				\
> +	dec	reg;				\
> +	jnz	.Ldo_call1_ ## uniq;		\
> +	add	$(BITS_PER_LONG/8) * nr, sp; \
> +.Lskip_rsb_ ## uniq:;
> +
> +
>  #ifdef __ASSEMBLY__
>  
>  /*
> @@ -61,6 +105,19 @@
>  #endif
>  .endm
>  
> + /*
> +  * A simpler FILL_RETURN_BUFFER macro. Don't make people use the CPP
> +  * monstrosity above, manually.
> +  */
> +.macro FILL_RETURN_BUFFER reg:req nr:req ftr:req
> +#ifdef CONFIG_RETPOLINE
> +	 ALTERNATIVE "jmp .Lskip_rsb_\@",				\
> +		     __stringify(__FILL_RETURN_BUFFER_PREP(\reg,\nr)),	\
> +		     \ftr
> +	__FILL_RETURN_BUFFER(\reg,\nr,%_ASM_SP,\@)
> +#endif
> +.endm

This seems weird.  I liked v1 a lot better.  What's the problem with
patching in the whole thing?

Also, if you go back to v1, it should be an easy objtool fix, just add
ANNOTATE_NOSPEC_ALTERNATIVE in front of it.

-- 
Josh