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

From: Josh Poimboeuf
Date: Thu Jan 11 2018 - 09:21:11 EST


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