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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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
>