Re: [PATCH 4/4] x86/cpu-policy: Derive {,R}RSBA for guest policies

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/05/2023 10:40 am, Jan Beulich wrote:
> On 26.05.2023 13:06, Andrew Cooper wrote:
>> The RSBA bit, "RSB Alternative", means that the RSB may use alternative
>> predictors when empty.  From a practical point of view, this mean "Retpoline
>> not safe".
>>
>> Enhanced IBRS (officially IBRS_ALL in Intel's docs, previously IBRS_ATT) is a
>> statement that IBRS is implemented in hardware (as opposed to the form
>> retrofitted to existing CPUs in microcode).
>>
>> The RRSBA bit, "Restricted-RSBA", is a combination of RSBA, and the eIBRS
>> property that predictions are tagged with the mode in which they were learnt.
>> Therefore, it means "when eIBRS is active, the RSB may fall back to
>> alternative predictors but restricted to the current prediction mode".  As
>> such, it's stronger statement than RSBA, but still means "Retpoline not 
>> safe".
> Just for my own understanding: Whether retpoline is safe with RRSBA does
> depend on the level of control a less privileged entity has over a more
> privileged entity's alternative predictor state?

Correct, but...

> If so, maybe add "probably" to the quoted statement?

... Spectre-BHI proved it was exploitable and could leak data.

"Don't do JIT in the kernel" was a very unsatisfactory resolution, and
in particular I think there is room to replicate the exploit with array
style sys/hypercalls.

As far as I'm concerned it's a matter of when, not if, a researcher
breaks this boundary again.

Concern in this area is why Intel added
MSR_SPEC_CTRL.{RRSBA,BHI}_DIS_{U,S} controls in ADL/SPR, which hobbles
this behaviour.  (And yes, we need to support these in guests too, but
that involves rearranging Xen's MSR_SPEC_CTRL handling which is why I
haven't gotten around to it yet.)

>> --- a/xen/arch/x86/cpu-policy.c
>> +++ b/xen/arch/x86/cpu-policy.c
>> @@ -423,8 +423,14 @@ static void __init 
>> guest_common_max_feature_adjustments(uint32_t *fs)
>>           * Retpoline not safe)", so these need to be visible to a guest in 
>> all
>>           * cases, even when it's only some other server in the pool which
>>           * suffers the identified behaviour.
>> +         *
>> +         * We can always run any VM which has previously (or will
>> +         * subsequently) run on hardware where Retpoline is not safe.  Note:
>> +         * The dependency logic may hide RRSBA for other reasons.
>>           */
>>          __set_bit(X86_FEATURE_ARCH_CAPS, fs);
>> +        __set_bit(X86_FEATURE_RSBA, fs);
>> +        __set_bit(X86_FEATURE_RRSBA, fs);
>>      }
>>  }
> Similar question to what I raised before: Can't this lead to both bits being
> set, which according to descriptions earlier in the series and elsewhere
> ought to not be possible?

In this case, no, because the max values are fully discarded when
establishing the default policy.

Remember this value is used to decide whether an incoming VM can be
accepted.  It doesn't reflect a sensible configuration to run.

Whether or not both values ought to be visible is the subject of the
outstanding question.

>
>> --- a/xen/tools/gen-cpuid.py
>> +++ b/xen/tools/gen-cpuid.py
>> @@ -318,7 +318,7 @@ def crunch_numbers(state):
>>          # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating 
>> them
>>          # as dependent features simplifies Xen's logic, and prevents the 
>> guest
>>          # from seeing implausible configurations.
>> -        IBRSB: [STIBP, SSBD, INTEL_PSFD],
>> +        IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS],
> Is this really an architecturally established dependency? From an abstract
> pov having just eIBRS ought to be enough of an indicator?

This is the same as asking "can we hide AVX512F but expose AVX512_*"...

> And hence it would
> be wrong to hide it just because IBRSB isn't also set.

EIBRS means "you should set MSR_SPEC_CTRL.IBRS once at the start of day
and leave it set", which to me firmly states a dependency.


>  Plus aiui ...
>
>> @@ -328,6 +328,9 @@ def crunch_numbers(state):
>>  
>>          # The ARCH_CAPS CPUID bit enumerates the availability of the whole 
>> register.
>>          ARCH_CAPS: list(range(RDCL_NO, RDCL_NO + 64)),
>> +
>> +        # The behaviour described by RRSBA depend on eIBRS being active.
>> +        EIBRS: [RRSBA],
> ... for the purpose of the change here this dependency is all you need.

This change is to make the values sane for a guest, which includes "you
don't get RRSBA, or EIBRS if you have to level IBRS out".

~Andrew