[1/2] xen/arm: entry: Place a speculation barrier following an ret instruction
diff mbox series

Message ID 20200616175913.7368-2-julien@xen.org
State New
Headers show
Series
  • xen/arm: Mitigate straight-line speculation
Related show

Commit Message

Julien Grall June 16, 2020, 5:59 p.m. UTC
From: Julien Grall <jgrall@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 the guest.

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@amazon.com>

---

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.
---
 xen/arch/arm/arm32/entry.S | 1 +
 xen/arch/arm/arm64/entry.S | 2 ++
 2 files changed, 3 insertions(+)

Comments

Stefano Stabellini June 16, 2020, 9:24 p.m. UTC | #1
On Tue, 16 Jun 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@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 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@amazon.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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
>
Julien Grall June 17, 2020, 4:23 p.m. UTC | #2
Hi,

On 16/06/2020 22:24, Stefano Stabellini wrote:
> On Tue, 16 Jun 2020, Julien Grall wrote:
>> From: Julien Grall <jgrall@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 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@amazon.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 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/.
 From my understanding, the compiler will add a speculation barrier 
preventively after each 'ret' when the mitigation are turned on.So it 
feels to me we want to follow the same approach.

Obviously, we can avoid them but I would like to have a justification 
for not adding them (nothing is overkilled against speculation ;)).

Cheers,
Julien Grall July 4, 2020, 4:07 p.m. UTC | #3
On 17/06/2020 17:23, Julien Grall wrote:
> Hi,
> 
> On 16/06/2020 22:24, Stefano Stabellini wrote:
>> On Tue, 16 Jun 2020, Julien Grall wrote:
>>> From: Julien Grall <jgrall@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 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@amazon.com>
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> 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/.
>  From my understanding, the compiler will add a speculation barrier 
> preventively after each 'ret' when the mitigation are turned on.So it 
> feels to me we want to follow the same approach.
> 
> Obviously, we can avoid them but I would like to have a justification 
> for not adding them (nothing is overkilled against speculation ;)).

I finally found some time to look at arm*/lib in more details. Some of 
the helpers can definitely be called with guest inputs.

For instance, memchr() is called from hypfs_get_path_user() with the 3rd 
argument controlled by the guest. In both 32-bit and 64-bit 
implementation, you will reach the end of the function memchr() with 
r2/w2 and r3/w3 (it contains a character from the buffer) controlled by 
the guest.

As this is the only function in the unit, we don't know what will be the 
instructions right after RET. So it would be safer to add a speculation 
barrier there too.

Note that hypfs is currently only accessibly by Dom0. Yet, I still think 
we should try to harden any code if we can :).

Cheers,

Patch
diff mbox series

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: