From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien@xen.org>
Cc: sstabellini@kernel.org, paul@xen.org, Andre.Przywara@arm.com,
Julien Grall <jgrall@amazon.com>,
Bertrand.Marquis@arm.com, security@xenproject.org,
xen-devel@lists.xenproject.org, Volodymyr_Babchuk@epam.com
Subject: Re: [PATCH 2/2] xen/arm: Mitigate straight-line speculation for SMC call
Date: Tue, 16 Jun 2020 14:34:34 -0700 (PDT) [thread overview]
Message-ID: <alpine.DEB.2.21.2006161425480.24982@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <20200616175913.7368-3-julien@xen.org>
On Tue, 16 Jun 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> SMC call will update some of registers (typically only x0) depending on
^a SMC call
> the arguments provided.
>
> Some CPUs can speculate past a SMC instruction and potentially perform
> speculative access to emrmoy using the pre-call values before executing
^ memory
> the SMC.
>
> There is no known gadget available after the SMC call today. However
> some of the registers may contain values from the guest and are expected
> to be updated by the SMC call.
>
> In order to harden the code, it would be better to prevent straight-line
> speculation from an SMC. Architecturally executing the speculation
^ a? any?
> barrier after every SMC can be rather expensive (particularly on core
> not SB). Therefore we want to mitigate it diferrently:
^ not SB capable? ^ differently
>
> * For arm_smccc_1_0_smc, we can avoid a speculation barrier right
> after the SMC instruction and instead rely on the one after eret.
^ ret
> * For arm_smccc_1_1_smc, we can place a B instruction after the SMC
> instruction to skip the barrier.
>
> Note that arm_smccc_1_0_smc version on arm32 is just an alias to
> arm_smccc_1_1_smc.
>
> Note that no speculation barrier has been added after the SMC
> instruction in arm64/entry.S. This is fine because the call is not
> expected to modify any registers. So straight-line speculation doesn't
> matter.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
I couldn't spot any issues with the patch and I compile-tested it.
> ---
>
> Note this hasn't been vetted by Arm but they are using the same
> sort of mitigation for blr. So I am quite confident this could do the
> trick.
Noted
> ---
> xen/arch/arm/arm64/smc.S | 1 +
> xen/include/asm-arm/smccc.h | 13 +++++++++++++
> xen/include/asm-arm/system.h | 8 ++++++++
> 3 files changed, 22 insertions(+)
>
> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> index b0752be57e8f..e0a3106dd7ec 100644
> --- a/xen/arch/arm/arm64/smc.S
> +++ b/xen/arch/arm/arm64/smc.S
> @@ -30,3 +30,4 @@ ENTRY(__arm_smccc_1_0_smc)
> stp x2, x3, [x4, #SMCCC_RES_a2]
> 1:
> ret
> + sb
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 9d94beb3df2d..8650224923b1 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -200,11 +200,24 @@ struct arm_smccc_res {
> * We have an output list that is not necessarily used, and GCC feels
> * entitled to optimise the whole sequence away. "volatile" is what
> * makes it stick.
> + *
> + * Some of the SMC callers are passing directly values that are
> + * controlled by the guest. To mitigate against straight-line
> + * speculation, a speculation barrier is required. As it may be
> + * expensive to architecturally execute the speculation barrier, we are
> + * using a B instruction to architecturally skip it.
> + *
> + * Note that the speculation barrier is technically not necessary as the
> + * B instruction should already block straight-line speculation. But
> + * better be safe than sorry ;).
Eh eh indeed :-)
I think this would be one thing to consider removing depending on ARM's
feedback.
> */
> #define arm_smccc_1_1_smc(...) \
> do { \
> __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
> asm volatile("smc #0\n" \
> + "b 1f\n" \
> + ASM_SB \
> + "1:\n" \
> __constraints(__count_args(__VA_ARGS__))); \
> if ( ___res ) \
> *___res = (typeof(*___res)){r0, r1, r2, r3}; \
> diff --git a/xen/include/asm-arm/system.h b/xen/include/asm-arm/system.h
> index 65d5c8e423d7..e33ff4e0fc39 100644
> --- a/xen/include/asm-arm/system.h
> +++ b/xen/include/asm-arm/system.h
> @@ -33,6 +33,14 @@
> #define smp_mb__before_atomic() smp_mb()
> #define smp_mb__after_atomic() smp_mb()
>
> +/*
> + * Speculative barrier
> + * XXX: Add support for the 'sb' instruction
> + */
> +#define ASM_SB "dsb nsh \n isb \n"
Why not ';' ? Anyway it doesn't matter.
> +#define sb() asm volatile(ASM_SB)
> +
> /*
> * This is used to ensure the compiler did actually allocate the register we
> * asked it for some inline assembly sequences. Apparently we can't trust
> --
> 2.17.1
>
next prev parent reply other threads:[~2020-06-16 21:34 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-16 17:59 [PATCH 0/2] xen/arm: Mitigate straight-line speculation Julien Grall
2020-06-16 17:59 ` [PATCH 1/2] xen/arm: entry: Place a speculation barrier following an ret instruction Julien Grall
2020-06-16 21:24 ` Stefano Stabellini
2020-06-17 16:23 ` Julien Grall
2020-07-04 16:07 ` Julien Grall
2020-08-18 16:35 ` Bertrand Marquis
2020-08-18 16:43 ` Julien Grall
2020-08-18 17:06 ` Bertrand Marquis
2020-08-18 17:34 ` Julien Grall
2020-08-19 7:59 ` Bertrand Marquis
2020-08-19 8:02 ` Jan Beulich
2020-08-19 8:50 ` Julien Grall
2020-08-19 8:58 ` Jan Beulich
2020-08-19 9:56 ` Julien Grall
2020-06-16 17:59 ` [PATCH 2/2] xen/arm: Mitigate straight-line speculation for SMC call Julien Grall
2020-06-16 21:34 ` Stefano Stabellini [this message]
2020-06-16 21:57 ` Julien Grall
2020-06-16 23:16 ` Andrew Cooper
2020-06-16 23:27 ` Stefano Stabellini
2020-06-18 17:46 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.21.2006161425480.24982@sstabellini-ThinkPad-T480s \
--to=sstabellini@kernel.org \
--cc=Andre.Przywara@arm.com \
--cc=Bertrand.Marquis@arm.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=jgrall@amazon.com \
--cc=julien@xen.org \
--cc=paul@xen.org \
--cc=security@xenproject.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).