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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 26 Mar 2021, at 11:56, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 26/03/2021 11:13, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 20 Mar 2021, at 13:13, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> (+ Security)
>>> 
>>> 
>>> On 17/03/2021 14:56, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi Bertrand,
>>> 
>>>>> 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.
>>> 
>>> Thanks for testing! It is indeed a lot nicer and less error-prone. We can 
>>> thansk Jan for the idea as he originally introduced it on x86 :).
>>> 
>>>> I also think that the mitigation on arm32/v7 would be messy to do.
>>> 
>>> It is messy but not impossible :). Some of the assembly function could be 
>>> rewritten in C to take advantage of the compiler mitigations.
>>> 
>>> I went through the paper again today. Straight-line mitigation only refers 
>>> to unconditional control flow change (e.g. RET) on AArch64 Armv8.
>>> 
>>> A recent submission to LLVM seems to suggest that Armv7 and AArch32 Armv8 
>>> is also affected [2].
>> Thanks for the pointer :-)
>>> 
>>> So I think we only need to care of unconditional return instruction (e.g. 
>>> mov pc, lr).
>>> 
>>> For conditional return instructions, they would be treated as spectre v2 
>>> which we already mitigate.
>> That would be a good idea but that would mean lots of invasive changes on 
>> armv7 and
> It is not quite clear which change you think is invasive... The change for 
> adding a barrier after all unconditional return instruction is pretty 
> straight-forward.
> 
> Regarding conditional return instructions, then is nothing to do for 
> straight-line speculation.
> 
>> this is not the mostly tested architecture with Xen.
> To me this looks very subjective, how do you define "mostly tested"?
> 
> From Xen Project perspective, we run the same test suite on arm64 and arm32 
> multiple time daily. I couldn't say the same for some of the Arm drivers in 
> the tree.

All together i just want to say that I have no testing capacities for arm32 (in 
hardware and persons) and the “user base” for it might not be huge (but I might 
be wrong here).

If you think that there is enough testing for it available or other might be 
able to test it then no problem for me, I will help on the review side.

Cheers
Bertrand

> 
>> Anyway I am happy to help reviewing this if it is done.
>>> 
>>>> Shall we mark v7/aarch32 as not security supported ?
>>> This would have consequence beyond just speculation. There might be 
>>> processor out which are not affected by straight-line speculation and we 
>>> would not issue any security update for them. So I am not in favor with 
>>> this approach.
>>> 
>>> What we could consider is mentioning in SUPPORT.MD that speculation attack 
>>> using Straight-Line speculation are not security support on Arm (the 64-bit 
>>> is not fully mitigated).
>> Weird to say “not security supported” maybe saying not mitigated by Xen 
>> would be more clear.
> 
> I am open for the wording :).
> 
> Cheers,
> 
> -- 
> Julien Grall
>