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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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