Re: [PATCH 2/2] x86/svm: Keep the RAS balanced for guests

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

On 12/08/2022 08:29, Jan Beulich wrote:
> On 11.08.2022 21:59, Andrew Cooper wrote:
>> One source of lost performance was that fact that to protect Xen from a
>> malicious guests, we had to flush the RAS.
>>
>> It turns out that CET Shadow Stacks give us enough architectural guarantees to
>> construct a lower overhead mitigation, which keeps the RAS balanced for the
>> guest so their return performance is still good.
>>
>> To keep the RAS balanced, Xen must execute the same number of CALLs as RETs
>> across one VMexit->VMEntry.  Without CET-SS, we could achieve this fairly
>> easily with a `call; add $8, %rsp` and `push; ret` pair, but this is not legal
>> under CET-SS.  In fact, CALL is the only shadow stack "push" operation we
>> have, and we can't use it a second time if we intend to keep the RAS balanced.
>>
>> Instead, we keep a real return address on the stack.  This means that for some
>> of entry.S, %rsp conditionally doesn't reference CPUINFO.
>>
>> This necessitates swapping the current order of DO_OVERWRITE_RSB and
>> svm_vmexit_spec_ctrl; while they don't have any specific ordering
>> requirements, push_one_ras needs to come after svm_vmexit_spec_ctrl or else we
>> need some very invasive changes to fix up the %rsp changes.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>>
>> RFC for a couple of reasons.  This does function correctly, but I still want
>> to do more perf testing.
> As per further down you mean to say it functions correctly without the
> use of alternatives. And even then (see below) I suppose it doesn't
> function correctly with no (or unused) CET-SS but CONFIG_XEN_SHSTK=y.

I came to realise that after sending that, until we get nested
alternatives, I can't make it function correctly for the
conditionally-not-CET-SS case without doing the full mov/rdsspq/cmp
sequence.

While that's possible, I'm not inclined to add the size overhead for a
case I'm not sure is safe.

So, the alternative to turn this on is going to strictly depend on
CONFIG_XEN_SHSTK.

At some point when nested alternatives appear, we can relax the
requirement, if there is a desperate wish for the perf improvement in
uncertain safety conditions.

>
>> Secondly, X86_FEATURE_ALWAYS is clearly not ok for committing.  I'm still
>> debating whether to make this construct available in !CET-SS cases.
>> Mechanically, its fine, but the safety arguments depend on CET-SS being
>> active.
> I'm afraid it's not entirely clear what you mean here, nor why you've used
> X86_FEATURE_ALWAYS in the first place when we have X86_FEATURE_XEN_SHSTK.

It can't depend on X86_FEATURE_XEN_SHSTK because, like the PBRSB case on
Intel, I want a way for the user to switch back to stuff32 if it
subsequently turns out that I've screwed up in the safety reasoning.

> If "this construct" is push_one_ras, then the mere use of WRSSQ requires
> it to not be used based on a runtime characteristic, not just a build
> time one. Hence afaict you could as well put the entire macro body in the
> #ifdef that currently encloses only the CET-SS insns.

What I mean is that "this" is faster than stuff32, and might be
"acceptably safe" for someone either on BTC-vunerable hardware, or newer
hardware but with CET-SS explicitly turned off.

That said...

>> In principle, on CPUs which do not suffer Branch Type Confusion, you might be
>> able to reason a defence-in-depth argument that if an attacker can't control
>> indirect speculation, then they can't bypass the 1-stuff safety either, but
>> the only AMD CPUs not vulnerable to BTC have CET-SS anyway.
> Yet people may have reasons to turn off its use.

... I view this as a perf improvement only.

As such, I'm entirely happy to say that it's only available in the case
which is easy to do, ought to be the common case, and is the case which
I'm confident is safe.

If people want it in other cases too, they can see about helping nested
alternatives along...

>> Third, I'd like some early feedback on how clear it the logic is given the
>> conditional nature of %rsp not referencing CPUINFO.
> It's assembly code, touching of which needs extra care. Taking together
> the size of the entire file (quite small) and the comments you add, I'd
> say that's fine.

That's good.  It turned out to be far less invasive than I feared.

The major observation which allowed for this was that it doesn't matter
trying to keep the RAS balanced immediately after schedule, so we don't
need to play games with reset_stack_and_jump() to try and execute one
fewer CALL.

>> Fourth, the alternatives logic (I think) needs improving to not fix up a
>> direct CALL/JMP displacement if the destination is within the replacement
>> length.  I did the functional testing before wrapping things in alternatives.
> Yes, unless you want to prefix the CALL with a redundant insn prefix to
> hide it from the displacement adjustment logic.

Hmm, that should work, but is ugly.

Honestly, I think I'd prefer to take the take the time to start doing a
boot time self-test for alternatives, as pert the SMC testing
improvements, because we desperately need this for so many other reasons
too.

~Andrew