Re: [PATCH 1/2] xen/arm: entry: Place a speculation barrier following an ret instruction

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 16 Jun 2020, 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 the guest.
                              ^ by


> In order to harden the code, it would be better to add a speculation
> barrier after each RET instruction. The performance is meant to be
> negligeable as the speculation barrier is not meant to be archicturally
> executed.
> 
> Note that on arm32, the ldmia instruction will act as a return from the
> function __context_switch(). While the whitepaper doesn't suggest it is
> possible to speculate after the instruction, add preventively a
> speculation barrier after it as well.
> 
> This is part of the work to mitigate straight-line speculation.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

I did a compile-test on the patch too.


> ---
> 
> I am still unsure whether we preventively should add a speculation barrier
> preventively after all the RET instructions in arm*/lib/. The smc call be
> taken care in a follow-up patch.

SMC is great to have but it seems to be overkill to do the ones under
lib/.


> ---
>  xen/arch/arm/arm32/entry.S | 1 +
>  xen/arch/arm/arm64/entry.S | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index b228d44b190c..bd54fc43558b 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -442,6 +442,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/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 175ea2981e72..b37d8fe09dc2 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -522,6 +522,7 @@ abort_guest_exit_end:
>          cset    x19, ne
>  
>          ret
> +        sb
>  ENDPROC(check_pending_guest_serror)
>  
>  /*
> @@ -583,6 +584,7 @@ ENTRY(__context_switch)
>          ldr     lr, [x8]
>          mov     sp, x9
>          ret
> +        sb
>  
>  /*
>   * Local variables:
> -- 
> 2.17.1
>