Re: [PATCH v3 2/2] xen/arm64: Place a speculation barrier following an ret instruction

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 1 Apr 2021, Julien Grall 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>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> 
> ---
> 
> 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.
> The LLVM website suggests it is: https://reviews.llvm.org/D92395
> 
> For now only focus on arm64.
> 
>     Changes in v3:
>         -  Add Bertrand's reviewed-by
> 
>     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

This series is very nice Julien! You can add my acked-by to both patches
and also commit them.

One minor request: could you please replace this comment with

/* ret opcode */

on commit?  // is not part of the coding style.


> +        .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
>