Hi Julien,
> On 13 Mar 2021, at 16:06, Julien Grall <jul...@xen.org> wrote:
>
> From: Julien Grall <jgr...@amazon.com>
>
> 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 <jgr...@amazon.com>
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.marq...@arm.com>
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
>
