Re: [Xen-devel] [PATCH v6.5 20/26] x86: Protect unaware domains from meddling hyperthreads

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.01.18 at 15:21, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/01/18 09:59, Jan Beulich wrote:
>>>>> On 04.01.18 at 01:15, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Fundamentally (as before)
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> However:
>>
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -2027,6 +2027,25 @@ int domain_relinquish_resources(struct domain *d)
>>>   */
>>>  void cpuid_policy_updated(struct vcpu *v)
>>>  {
>>> +    const struct cpuid_policy *cp = v->domain->arch.cpuid;
>>> +    struct msr_vcpu_policy *vp = v->arch.msr;
>>> +
>>> +    /*
>>> +     * For guests which know about IBRS but are not told about STIBP 
> running
>>> +     * on hardware supporting hyperthreading, the guest doesn't know to
>>> +     * protect itself fully.  (Such a guest won't be permitted direct 
> access
>>> +     * to the MSR.)  Have Xen fill in the gaps, so an unaware guest can't 
> be
>>> +     * interfered with by a meddling guest on an adjacent hyperthread.
>>> +     */
>>> +    if ( cp->feat.ibrsb )
>>> +    {
>>> +        if ( !cp->feat.stibp && cpu_has_stibp &&
>>> +             !(vp->spec_ctrl.guest & (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP)) )
>>> +            vp->spec_ctrl.host = SPEC_CTRL_STIBP;
>>> +        else
>>> +            vp->spec_ctrl.host = vp->spec_ctrl.guest;
>> This code is so similar to ...
>>
>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -181,7 +181,20 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>>>                       (cp->feat.stibp ? SPEC_CTRL_STIBP : 0)) )
>>>              goto gp_fault; /* Rsvd bit set? */
>>>          vp->spec_ctrl.guest = val;
>>> -        vp->spec_ctrl.host  = val;
>>> +
>>> +        /*
>>> +         * For guests which are not told about STIBP, running on hardware
>>> +         * supporting hyperthreading, the guest doesn't know to protect 
>>> itself
>>> +         * fully.  (Such a guest won't be permitted direct access to the 
>>> MSR.)
>>> +         * When IBRS is not in force, have Xen fill in the gaps, so an 
>>> unaware
>>> +         * guest can't be interfered with by a meddling guest on an 
>>> adjacent
>>> +         * hyperthread.
>>> +         */
>>> +        if ( !cp->feat.stibp && cpu_has_stibp &&
>>> +             !(val & (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP)) )
>>> +            vp->spec_ctrl.host = SPEC_CTRL_STIBP;
>>> +        else
>>> +            vp->spec_ctrl.host = val;
>> ... this that I think a helper function would be warranted, unless you
>> have reasons to believe that future changes might break the
>> similarity.
> 
> I don't expect them to diverge, and will pull it out into a separate helper.
> 
>>
>> I'm also a little puzzled by you checking SPEC_CTRL_STIBP there -
>> this bit ought to be clear when !cp->feat.stibp due to the earlier
>> reserved bit check (part of which is even visible in context above).
>> IOW the check is not wrong, but perhaps misleading. You had
>> replied to this remark with
>>
>> "The SPEC_CTRL_STIBP check exists solely because of v3 review which
>>  objected to me implying a link between IBRS and STIPB."
> 
> The original logic was was "!cp->feat.stibp && cpu_has_stibp && val ==
> 0", which you argued would go stale as new SPEC_CTRL_ bits got added.

Ah, I recall now. But by just checking !(val & SPEC_CTRL_IBRS) you
would avoid the staleness; you might even consider putting an
ASSERT() in to validate the other bit is clear.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel