Hi, > On 26 Mar 2021, at 11:56, Julien Grall <jul...@xen.org> wrote: > > > > On 26/03/2021 11:13, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >>> On 20 Mar 2021, at 13:13, Julien Grall <jul...@xen.org> wrote: >>> >>> (+ Security) >>> >>> >>> On 17/03/2021 14:56, Bertrand Marquis wrote: >>>> Hi Julien, >>> >>> Hi Bertrand, >>> >>>>> On 13 Mar 2021, at 16:06, Julien Grall <jul...@xen.org> wrote: >>>>> >>>>> From: Julien Grall <jgr...@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 <jgr...@amazon.com> >>>> 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 >
