Re: [PATCH RESEND] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[Andrew Cooper]

On 20/02/2023 2:31 pm, Borislav Petkov wrote:
> Drop stable@.
>
> On Mon, Feb 20, 2023 at 04:34:02AM -0800, KP Singh wrote:
>>>> On Mon, Feb 20, 2023 at 01:01:27PM +0100, KP Singh wrote:
>>>>> +static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode)
>>>>> +{
>>>>> +     /* When IBRS or enhanced IBRS is enabled, STIBP is not needed.
>>>>> +      *
>>>>> +      * However, With KERNEL_IBRS, the IBRS bit is cleared on return
>>>>> +      * to user and the user-mode code needs to be able to enable protection
>>>>> +      * from cross-thread training, either by always enabling STIBP or
>>>>> +      * by enabling it via prctl.
>>>>> +      */
>>>>> +     return (spectre_v2_in_ibrs_mode(mode) &&
>>>>> +             !cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));
>>>>> +}
>>>> The comments and code confused me, they both seem to imply some
>>>> distinction between IBRS and KERNEL_IBRS, but in the kernel those are
>>>> functionally the same thing.  e.g., the kernel doesn't have a user IBRS
>>>> mode.
>>>>
>>>> And, unless I'm missing some subtlety here, it seems to be a convoluted
>>>> way of saying that eIBRS doesn't need STIBP in user space.
>> Actually, there is a subtlety, STIBP is not needed in IBRS and eIBRS
>> however, with KERNEL_IBRS we only enable IBRS (by setting and
>> unsetting the IBRS bit of SPEC_CTL) in the kernel context and this is
>> why we need to allow STIBP in userspace. If it were for pure IBRS, we
>> would not need it either (based on my reading). Now, with
>> spectre_v2_in_ibrs_mode, as per the current implementation implies
>> that KERNEL_IBRS is chosen, but this may change. Also, I would also
>> prefer to have it more readable in the sense that:
>>
>> "If the kernel decides to write 0 to the IBRS bit on returning to the
>> user, STIBP needs to to be allowed in user space"
> From:
>
> https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/indirect-branch-restricted-speculation.html
>
> "If IA32_SPEC_CTRL.IBRS is already 1 before a transition to a more
> privileged predictor mode, some processors may allow the predicted
> targets of indirect branches executed in that predictor mode to be
> controlled by software that executed before the transition. Software can
> avoid this by using WRMSR on the IA32_SPEC_CTRL MSR to set the IBRS bit
> to 1 after any such transition, regardless of the bit’s previous value.
> It is not necessary to clear the bit first; writing it with a value of
> 1 after the transition suffices, regardless of the bit’s original
> value."
>
> I'd love it if we could simplify our code by not caring of the IBRS bit
> when returning to user but I'm afraid that it ain't that simple.
>
> This above probably wants to say that you need to write 1 on CPL change
> because it has a flushing behavior of killing user prediction entries.
>
> Lemme add Andy and dhansen for clarification.
>

"When IBRS or enhanced IBRS is enabled, STIBP is not needed."

This is misleading, if not strictly wrong.  The IBRS bit being set
implies STIBP, which reads differently to "not needed".


Now - eIBRS is "set once at start of day" which ends up becoming a
global implicit STIBP.

I think we're discussing the legacy IBRS case here.  i.e. what was
retrofitted in microcode for existing parts?

The reason why it is "write 1 on each privilege increase, 0 on privilege
decrease" is because on some CPUs its an inhibit control, and on some
CPUs is a flush (i.e. its actually IBPB).

But these same CPUs don't actually have an ability to thread-tag the
indirect predictor nicely so STIBP is also horribly expensive under the
hood - so much so that we were firmly recommended to clear STIBP/IBRS
when going idle so as to reduce the impact on the sibling.


IMO the proper way to do this is to set STIBP uniformly depending on
whether you want it in userspace or not, and treat it logically
separately to IBRS.  It doesn't hurt (any more) to have both bits set.

~Andrew