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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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