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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

(sorry for the delay)

> 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 this is not the mostly tested architecture with Xen.
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.

Anyway I agree with the principle to do this.

Cheers
Bertrand

> 
> Cheers,
> 
> [1] https://reviews.llvm.org/D92395
> 
> -- 
> Julien Grall