[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction


  • To: Julien Grall <julien@xxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Wed, 17 Mar 2021 14:56:25 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=RlbL7v8bpY9VAQR/1LAtAHwMNa+Ynp8uqeKiEXogAXo=; b=J4ddzLOeIrVvLqwr2XMrI1S2fRy1KA4bA6HbuyIDik5SIns9xkCljgDvHljjsyMS1P17ugqmQk482yg4WoexCjMPM1B4uZWeU9AUp8dvlCMGeu4y0z/4tgWiCNbbZgyph9lDzxg/wExeSEcJRiTmprbcOn8uRiv3W+XtBattRLc7Ea9y/EqBN30sCJECfdCj1KOYXWm8exkI3Di/VlvZMQtcgO871bd1qmlzphWmu/9lxbq7UalgRcuT9gpCQblo4u/tHVyxXfc5NbqB9GUi1X6z6H+Nq77W4RGFnmTtSxOfWEIK2MGR3AWTyKQh69YhBi6FRR+vw9Mgfg2R3TopRw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=L3oac2ajcMGQygxs8Vdi2pMuovluolsT+nJbKUeGLtSkCWmFvZfKTjU5DZwh/WxuKFvN8e9QRvWK+0QiqCsP3hCaF4O+9/4JbN5MD25NbdbEu1xboFFgLafCckEfX5FWlgxvfrxNiKTxlAJ2QHMj1Os9w411hGvRob9K3hzODlc2kxVMXBPysvjvZTnj+Y38Y1P9s1LjCWgGT79sRlVh9MvJNOO1oRS9bTyheZMZXFPxnINuvMNGOrKREE8OivbuNrTBXQCL+i2uycOZv6aYbWwkKJnlWJ9OVIoCFCYcB3VU5pzbZBCq0dBL+n1t534woQNCybSOBUeyw24LAJNMrg==
  • Authentication-results-original: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Julien Grall <jgrall@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 17 Mar 2021 14:56:42 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHXGCLUePNZXth41EOpDgjR62uDxqqISyAA
  • Thread-topic: [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction

Hi Julien,

> On 13 Mar 2021, at 16:06, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Some CPUs can speculate past a RET instruction and potentially perform
> speculative accesses to memory before processing the return.
> 
> There is no known gadget available after the RET instruction today.
> However some of the registers (such as in check_pending_guest_serror())
> may contain a value provided by the guest.
> 
> In order to harden the code, it would be better to add a speculation
> barrier after each RET instruction. The performance impact is meant to
> be negligeable as the speculation barrier is not meant to be
> architecturally executed.
> 
> Rather than manually inserting a speculation barrier, use a macro
> which overrides the mnemonic RET and replace with RET + SB. We need to
> use the opcode for RET to prevent any macro recursion.
> 
> This patch is only covering the assembly code. C code would need to be
> covered separately using the compiler support.
> 
> This is part of the work to mitigate straight-line speculation.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

The macro solution is definitely a great improvement compared to v1 and I could
confirm the presence of the sb in the generated code.

I also think that the mitigation on arm32/v7 would be messy to do.
Shall we mark v7/aarch32 as not security supported ?

Apart from this global question (which does not need to be answered in this 
serie):

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Cheers
Bertrand


> 
> ---
> 
> It is not clear to me whether Armv7 (we don't officially support 32-bit
> hypervisor on Armv8) is also affected by straight-line speculation.
> 
> But the mitigation is a lot messier because opcode can be optionally
> executed. So this Arm32 is left alone for now.
> 
>    Changes in v2:
>        - Use a macro rather than inserting the speculation barrier
>        manually
>        - Remove mitigation for arm32
> ---
> xen/arch/arm/arm32/entry.S         |  1 +
> xen/arch/arm/arm32/lib/lib1funcs.S |  1 +
> xen/include/asm-arm/arm64/macros.h |  6 ++++++
> xen/include/asm-arm/macros.h       | 18 +++++++++---------
> 4 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index f2f1bc7a3158..d0a066484f13 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -441,6 +441,7 @@ ENTRY(__context_switch)
> 
>         add     r4, r1, #VCPU_arch_saved_context
>         ldmia   r4, {r4 - sl, fp, sp, pc}       /* Load registers and return 
> */
> +        sb
> 
> /*
>  * Local variables:
> diff --git a/xen/arch/arm/arm32/lib/lib1funcs.S 
> b/xen/arch/arm/arm32/lib/lib1funcs.S
> index f1278bd6c139..8c33ffbbcc4c 100644
> --- a/xen/arch/arm/arm32/lib/lib1funcs.S
> +++ b/xen/arch/arm/arm32/lib/lib1funcs.S
> @@ -382,5 +382,6 @@ UNWIND(.save {lr})
>       bl      __div0
>       mov     r0, #0                  @ About as wrong as it could be.
>       ldr     pc, [sp], #8
> +     sb
> UNWIND(.fnend)
> ENDPROC(Ldiv0)
> diff --git a/xen/include/asm-arm/arm64/macros.h 
> b/xen/include/asm-arm/arm64/macros.h
> index f981b4f43e84..4614394b3dd5 100644
> --- a/xen/include/asm-arm/arm64/macros.h
> +++ b/xen/include/asm-arm/arm64/macros.h
> @@ -21,6 +21,12 @@
>     ldr     \dst, [\dst, \tmp]
>     .endm
> 
> +    .macro  ret
> +        // ret opcode
> +        .inst 0xd65f03c0
> +        sb
> +    .endm
> +
> /*
>  * Register aliases.
>  */
> diff --git a/xen/include/asm-arm/macros.h b/xen/include/asm-arm/macros.h
> index 4833671f4ced..1aa373760f98 100644
> --- a/xen/include/asm-arm/macros.h
> +++ b/xen/include/asm-arm/macros.h
> @@ -5,6 +5,15 @@
> # error "This file should only be included in assembly file"
> #endif
> 
> +    /*
> +     * Speculative barrier
> +     * XXX: Add support for the 'sb' instruction
> +     */
> +    .macro sb
> +    dsb nsh
> +    isb
> +    .endm
> +
> #if defined (CONFIG_ARM_32)
> # include <asm/arm32/macros.h>
> #elif defined(CONFIG_ARM_64)
> @@ -20,13 +29,4 @@
>     .endr
>     .endm
> 
> -    /*
> -     * Speculative barrier
> -     * XXX: Add support for the 'sb' instruction
> -     */
> -    .macro sb
> -    dsb nsh
> -    isb
> -    .endm
> -
> #endif /* __ASM_ARM_MACROS_H */
> -- 
> 2.17.1
> 




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.