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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> 
> 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.

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

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

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

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

Jan