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

[Julien Grall <julien@xxxxxxx>]

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.
> 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