Re: [Xen-devel] [PATCH v3 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.07.18 at 11:37, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 27/07/18 10:28, Jan Beulich wrote:
>>>>> On 19.07.18 at 13:44, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> It turns out that Xen has never enforced that a domain remain within the
>>> xstate features advertised in CPUID.
>>>
>>> The check of new_bv against xfeature_mask ensures that a domain stays within
>>> the set of features that Xen has enabled in hardware (and therefore isn't a
>>> security problem), but this does means that attempts to level a guest for
>>> migration safety might not be effective if the guest ignores CPUID.
>>>
>>> Check the CPUID policy in validate_xstate() (for incoming migration) and in
>>> handle_xsetbv() (for guest XSETBV instructions).  This subsumes the PKRU 
> check
>>> for PV guests in handle_xsetbv() (and also demonstrates that I should have
>>> spotted this problem while reviewing c/s fbf9971241f).
>>>
>>> For migration, this is correct despite the current (mis)ordering of data
>>> because d->arch.cpuid is the applicable max policy.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>
>>> v2:
>>>  * Leave valid_xcr0() alone and duplicate the checks in validate_xstate() 
> and
>>>    handle_xsetbv().
>>> v3:
>>>  * Note the migration safety in the commit message.
>>>
>>> Backporting notes: This is safe in the restore case, but only back as far as
>>> the introduction of cpuid_policy infrastructure.  Before then, a restore
>>> boolean needs to be plumbed down as well, and used to select between the
>>> hardware maximum value and calls to {hvm,pv}_cpuid() to find the
>>> toolstack-chosen level.
>> While trying to determine the exact boundary here (looks like it's
>> between 4.7 and 4.8, in which case the remark is relevant only for
>> people maintaining releases no longer fully XenProject maintained)
>> I've become confused by the reference to {hvm,pv}_cpuid() above:
>> Is this simply an ordering concern? If so, the bounding the two
>> functions do would need to be replicated (or better shared) I think,
>> if the sole reason for otherwise using the HW maximum is that
>> d->arch.cpuids[] isn't populated yet.
> 
> Looking over things, 4.9 is fine because that was when cpuid_policy was
> introduced.
> 
> Before 4.9, the calls to {hvm,pv}_cpuid() are needed to because the
> information can't be read directly out of d->arch.cpuids[].  The restore
> boolean is needed because this array will be empty at the time it is
> accessed on the restore path.

If at restore time we were to not obey to the restrictions {hvm,pv}_cpuid()
enforce, there's imo no point doing any checks at all - the check against
the HW maximum exists already anyway.

Jan



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