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

[Julien Grall <julien@xen.org>]

Hi Stefano,

On 14/04/2021 00:03, Stefano Stabellini wrote:
> On Thu, 1 Apr 2021, 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 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@amazon.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> ---
>>
>> 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.

I have realized that I left a couple of arm32 specific code in the 
patch. I will commit the first one and respin this one.

> 
> One minor request: could you please replace this comment with
> 
> /* ret opcode */
> 
> on commit?  // is not part of the coding style.

I have modified it in the new version.

Cheers,

-- 
Julien Grall