Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Andrew Cooper]

On 05/09/16 10:51, Jan Beulich wrote:
 On 05.09.16 at 11:43,  wrote:
>> On 05/09/16 07:32, Jan Beulich wrote:
>> On 02.09.16 at 17:14,  wrote:
 On 01/09/16 16:27, Jan Beulich wrote:
>>> +{
>>> +if ( d->arch.x86_vendor == X86_VENDOR_AMD )
>>> +{
>>> +*eax = 0;
>>> +*ebx = 0;
>>> +*ecx = 0;
>>> +*edx = 0;
>>> +return;
>>> +}
>>> +if ( input >> 16 )
>>> +hvm_cpuid(0, , NULL, NULL, NULL);
>> Is this really the right way round?  The AMD method of "reserved always
>> as zero" is the more sane default to take.
> If anything I'd then say let's _always_ follow the AMD model.
 It would certainly be better to default to AMD, and special case others
 on an as-needed basis.

 Strictly speaking, following the AMD model is compatible with the
 "Reserved" nature specified for Intel.

 Lets just go with this.
>>> Done. But before sending v2, just to be clear: The group check
>>> which you also didn't like won't go away. That's because if we didn't
>>> do it, we'd hide all CPUID info outside the basic and extended group,
>>> in particular (in case we run virtualized ourselves) and leaves coming
>>> from the lower level hypervisor (most notably our own ones, if it's
>>> another Xen underneath).
>> Architecturally speaking, it is not ok for any of our guests to be able
>> to see our hypervisors leaves.
> I'm not convinced, and even less so by the generalization to groups
> like the Cyrix/VIA or Transmeta ones. Yes, some of the leaves there
> would likely need white listing to be applied just like what we do for
> some of the base and extended leaves. But even for those (base
> and extended) we don't hide everything (in particular we don't hide
> the respective lowest numbered subleaf), and hence I don't see why
> we should do so with all of the other groups.

Xen's handling is still broken (albeit it less than it used to be).  A
guest running under Xen should not be able to find any information not
explicitly controlled by Xen, because information leakage like this
causes problems on migrate, etc.

If at some point in the future we choose to extend the whitelist to
include new groups, that is fine.  However, groups which aren't already
explicitly handled by Xen should be excluded from guest view.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Jan Beulich]

>>> On 05.09.16 at 11:43,  wrote:
> On 05/09/16 07:32, Jan Beulich wrote:
> On 02.09.16 at 17:14,  wrote:
>>> On 01/09/16 16:27, Jan Beulich wrote:
>> +{
>> +if ( d->arch.x86_vendor == X86_VENDOR_AMD )
>> +{
>> +*eax = 0;
>> +*ebx = 0;
>> +*ecx = 0;
>> +*edx = 0;
>> +return;
>> +}
>> +if ( input >> 16 )
>> +hvm_cpuid(0, , NULL, NULL, NULL);
> Is this really the right way round?  The AMD method of "reserved always
> as zero" is the more sane default to take.
 If anything I'd then say let's _always_ follow the AMD model.
>>> It would certainly be better to default to AMD, and special case others
>>> on an as-needed basis.
>>>
>>> Strictly speaking, following the AMD model is compatible with the
>>> "Reserved" nature specified for Intel.
>>>
>>> Lets just go with this.
>> Done. But before sending v2, just to be clear: The group check
>> which you also didn't like won't go away. That's because if we didn't
>> do it, we'd hide all CPUID info outside the basic and extended group,
>> in particular (in case we run virtualized ourselves) and leaves coming
>> from the lower level hypervisor (most notably our own ones, if it's
>> another Xen underneath).
> 
> Architecturally speaking, it is not ok for any of our guests to be able
> to see our hypervisors leaves.

I'm not convinced, and even less so by the generalization to groups
like the Cyrix/VIA or Transmeta ones. Yes, some of the leaves there
would likely need white listing to be applied just like what we do for
some of the base and extended leaves. But even for those (base
and extended) we don't hide everything (in particular we don't hide
the respective lowest numbered subleaf), and hence I don't see why
we should do so with all of the other groups.

Jan

> The current call to cpuid_hypervisor_leaves() will clobber information
> from the outer hypervisor anyway (although I observe that dom0 running
> under Xen under Xen can observe the outer Xen leaves if L1 is configured
> with both Xen+Viridian).
> 
> I will be fixing this information leakage.  As for these bounds checks,
> I would put the checks after the cpuid_hypervisor_leaves() call, and
> exclude everything other than the base and extended groups.
> 
> ~Andrew




___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Andrew Cooper]

On 05/09/16 07:32, Jan Beulich wrote:
 On 02.09.16 at 17:14,  wrote:
>> On 01/09/16 16:27, Jan Beulich wrote:
> +{
> +if ( d->arch.x86_vendor == X86_VENDOR_AMD )
> +{
> +*eax = 0;
> +*ebx = 0;
> +*ecx = 0;
> +*edx = 0;
> +return;
> +}
> +if ( input >> 16 )
> +hvm_cpuid(0, , NULL, NULL, NULL);
 Is this really the right way round?  The AMD method of "reserved always
 as zero" is the more sane default to take.
>>> If anything I'd then say let's _always_ follow the AMD model.
>> It would certainly be better to default to AMD, and special case others
>> on an as-needed basis.
>>
>> Strictly speaking, following the AMD model is compatible with the
>> "Reserved" nature specified for Intel.
>>
>> Lets just go with this.
> Done. But before sending v2, just to be clear: The group check
> which you also didn't like won't go away. That's because if we didn't
> do it, we'd hide all CPUID info outside the basic and extended group,
> in particular (in case we run virtualized ourselves) and leaves coming
> from the lower level hypervisor (most notably our own ones, if it's
> another Xen underneath).

Architecturally speaking, it is not ok for any of our guests to be able
to see our hypervisors leaves.

The current call to cpuid_hypervisor_leaves() will clobber information
from the outer hypervisor anyway (although I observe that dom0 running
under Xen under Xen can observe the outer Xen leaves if L1 is configured
with both Xen+Viridian).

I will be fixing this information leakage.  As for these bounds checks,
I would put the checks after the cpuid_hypervisor_leaves() call, and
exclude everything other than the base and extended groups.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Jan Beulich]

>>> On 02.09.16 at 17:14,  wrote:
> On 01/09/16 16:27, Jan Beulich wrote:
>>
 +{
 +if ( d->arch.x86_vendor == X86_VENDOR_AMD )
 +{
 +*eax = 0;
 +*ebx = 0;
 +*ecx = 0;
 +*edx = 0;
 +return;
 +}
 +if ( input >> 16 )
 +hvm_cpuid(0, , NULL, NULL, NULL);
>>> Is this really the right way round?  The AMD method of "reserved always
>>> as zero" is the more sane default to take.
>> If anything I'd then say let's _always_ follow the AMD model.
> 
> It would certainly be better to default to AMD, and special case others
> on an as-needed basis.
> 
> Strictly speaking, following the AMD model is compatible with the
> "Reserved" nature specified for Intel.
> 
> Lets just go with this.

Done. But before sending v2, just to be clear: The group check
which you also didn't like won't go away. That's because if we didn't
do it, we'd hide all CPUID info outside the basic and extended group,
in particular (in case we run virtualized ourselves) and leaves coming
from the lower level hypervisor (most notably our own ones, if it's
another Xen underneath).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Andrew Cooper]

On 01/09/16 16:27, Jan Beulich wrote:
>
>>> +{
>>> +if ( d->arch.x86_vendor == X86_VENDOR_AMD )
>>> +{
>>> +*eax = 0;
>>> +*ebx = 0;
>>> +*ecx = 0;
>>> +*edx = 0;
>>> +return;
>>> +}
>>> +if ( input >> 16 )
>>> +hvm_cpuid(0, , NULL, NULL, NULL);
>> Is this really the right way round?  The AMD method of "reserved always
>> as zero" is the more sane default to take.
> If anything I'd then say let's _always_ follow the AMD model.

It would certainly be better to default to AMD, and special case others
on an as-needed basis.

Strictly speaking, following the AMD model is compatible with the
"Reserved" nature specified for Intel.

Lets just go with this.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Jan Beulich]

>>> On 01.09.16 at 16:27,  wrote:
> On 24/08/16 16:31, Jan Beulich wrote:
>> Another place where we should try to behave like real hardware; see
>> the code comments.
>>
>> Signed-off-by: Jan Beulich 
>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3358,6 +3358,31 @@ void hvm_cpuid(unsigned int input, unsig
>>  if ( !edx )
>>  edx = 
>>  
>> +if ( input & 0x )
>> +{
>> +/*
>> + * Requests beyond the highest supported leaf within a group return
>> + * zero on AMD and the highest basic leaf output on others.
>> + */
>> +unsigned int lvl;
>> +
>> +hvm_cpuid(input & 0x, , NULL, NULL, NULL);
>> +if ( ((lvl ^ input) >> 16) || input > lvl )
> 
> This logic isn't correct.  It doesn't cope in the Intel case when lvl
> aliases the upper 16 bits of input, despite input being an unknown group.
> 
> When I considered the problem before, the only functioning logic I came
> up with was to know that for Intel, input = 0x8000 is the only
> special case which doesn't collapse into the highest basic leaf.

If we had followed to Intel model before the extended leaf got
introduced, we'd have been incompatible with the extended leaf.
If ever someone introduces another group, we'd then again be
screwed. Yes, we can follow the Intel model, but I intentionally
tried to make the code more forward compatible.

>> +{
>> +if ( d->arch.x86_vendor == X86_VENDOR_AMD )
>> +{
>> +*eax = 0;
>> +*ebx = 0;
>> +*ecx = 0;
>> +*edx = 0;
>> +return;
>> +}
>> +if ( input >> 16 )
>> +hvm_cpuid(0, , NULL, NULL, NULL);
> 
> Is this really the right way round?  The AMD method of "reserved always
> as zero" is the more sane default to take.

If anything I'd then say let's _always_ follow the AMD model.

> I have looked at the Transmeta and Cyrix CPUID docs, and they are
> non-specific as to what reserved means.

While the Cyrix box I have doesn't stay up long enough anymore
to try out in practice, I do recall that in various aspects they very
closely followed what Intel does. There not being any 64-bit
Transmeta CPUs we support, I don't think we currently care about
their behavior.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Andrew Cooper]

On 24/08/16 16:31, Jan Beulich wrote:
> Another place where we should try to behave like real hardware; see
> the code comments.
>
> Signed-off-by: Jan Beulich 
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3358,6 +3358,31 @@ void hvm_cpuid(unsigned int input, unsig
>  if ( !edx )
>  edx = 
>  
> +if ( input & 0x )
> +{
> +/*
> + * Requests beyond the highest supported leaf within a group return
> + * zero on AMD and the highest basic leaf output on others.
> + */
> +unsigned int lvl;
> +
> +hvm_cpuid(input & 0x, , NULL, NULL, NULL);
> +if ( ((lvl ^ input) >> 16) || input > lvl )

This logic isn't correct.  It doesn't cope in the Intel case when lvl
aliases the upper 16 bits of input, despite input being an unknown group.

When I considered the problem before, the only functioning logic I came
up with was to know that for Intel, input = 0x8000 is the only
special case which doesn't collapse into the highest basic leaf.

> +{
> +if ( d->arch.x86_vendor == X86_VENDOR_AMD )
> +{
> +*eax = 0;
> +*ebx = 0;
> +*ecx = 0;
> +*edx = 0;
> +return;
> +}
> +if ( input >> 16 )
> +hvm_cpuid(0, , NULL, NULL, NULL);

Is this really the right way round?  The AMD method of "reserved always
as zero" is the more sane default to take.

I have looked at the Transmeta and Cyrix CPUID docs, and they are
non-specific as to what reserved means.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Andrew Cooper]

On 01/09/16 13:56, Jan Beulich wrote:
 On 01.09.16 at 13:23,  wrote:
>> On 24/08/16 16:31, Jan Beulich wrote:
>>> Another place where we should try to behave like real hardware; see
>>> the code comments.
>>>
>>> Signed-off-by: Jan Beulich 
>>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3358,6 +3358,31 @@ void hvm_cpuid(unsigned int input, unsig
>>>  if ( !edx )
>>>  edx = 
>>>  
>>> +if ( input & 0x )
>>> +{
>>> +/*
>>> + * Requests beyond the highest supported leaf within a group return
>>> + * zero on AMD and the highest basic leaf output on others.
>>> + */
>>> +unsigned int lvl;
>>> +
>>> +hvm_cpuid(input & 0x, , NULL, NULL, NULL);
>> I have specifically deferred fixing this issue so far, because I don't
>> want to increase the quantity of recursion with hvm_cpuid().
>>
>> Also, because of the poor datastructure for domain cpuid, this adds 1
>> and possibly 2 extra loops over the unordered list.
>>
>>
>> On the way back from Toronto, I started experimenting with my
>> full-policy plans, including a structured information layout so
>> cpuid.basic.max_leaf can be found directly, and starting a guest_cpuid()
>> function intending to replace both pv_cpuid() and hvm_cpuid() in due course.
>>
>> Would you be amenable to leaving this issue as-is for now, until there
>> is a more efficient way of fixing it?
> If you get this ready for 4.8, yes. Otherwise I think the variant here
> is better than nothing until yours arrives.

There is no way it will be done for 4.8.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Jan Beulich]

>>> On 01.09.16 at 13:23,  wrote:
> On 24/08/16 16:31, Jan Beulich wrote:
>> Another place where we should try to behave like real hardware; see
>> the code comments.
>>
>> Signed-off-by: Jan Beulich 
>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3358,6 +3358,31 @@ void hvm_cpuid(unsigned int input, unsig
>>  if ( !edx )
>>  edx = 
>>  
>> +if ( input & 0x )
>> +{
>> +/*
>> + * Requests beyond the highest supported leaf within a group return
>> + * zero on AMD and the highest basic leaf output on others.
>> + */
>> +unsigned int lvl;
>> +
>> +hvm_cpuid(input & 0x, , NULL, NULL, NULL);
> 
> I have specifically deferred fixing this issue so far, because I don't
> want to increase the quantity of recursion with hvm_cpuid().
> 
> Also, because of the poor datastructure for domain cpuid, this adds 1
> and possibly 2 extra loops over the unordered list.
> 
> 
> On the way back from Toronto, I started experimenting with my
> full-policy plans, including a structured information layout so
> cpuid.basic.max_leaf can be found directly, and starting a guest_cpuid()
> function intending to replace both pv_cpuid() and hvm_cpuid() in due course.
> 
> Would you be amenable to leaving this issue as-is for now, until there
> is a more efficient way of fixing it?

If you get this ready for 4.8, yes. Otherwise I think the variant here
is better than nothing until yours arrives.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Andrew Cooper]

On 24/08/16 16:31, Jan Beulich wrote:
> Another place where we should try to behave like real hardware; see
> the code comments.
>
> Signed-off-by: Jan Beulich 
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3358,6 +3358,31 @@ void hvm_cpuid(unsigned int input, unsig
>  if ( !edx )
>  edx = 
>  
> +if ( input & 0x )
> +{
> +/*
> + * Requests beyond the highest supported leaf within a group return
> + * zero on AMD and the highest basic leaf output on others.
> + */
> +unsigned int lvl;
> +
> +hvm_cpuid(input & 0x, , NULL, NULL, NULL);

I have specifically deferred fixing this issue so far, because I don't
want to increase the quantity of recursion with hvm_cpuid().

Also, because of the poor datastructure for domain cpuid, this adds 1
and possibly 2 extra loops over the unordered list.


On the way back from Toronto, I started experimenting with my
full-policy plans, including a structured information layout so
cpuid.basic.max_leaf can be found directly, and starting a guest_cpuid()
function intending to replace both pv_cpuid() and hvm_cpuid() in due course.

Would you be amenable to leaving this issue as-is for now, until there
is a more efficient way of fixing it?

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH] x86: correct CPUID output for out of bounds input

[Jan Beulich]

Another place where we should try to behave like real hardware; see
the code comments.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3358,6 +3358,31 @@ void hvm_cpuid(unsigned int input, unsig
 if ( !edx )
 edx = 
 
+if ( input & 0x )
+{
+/*
+ * Requests beyond the highest supported leaf within a group return
+ * zero on AMD and the highest basic leaf output on others.
+ */
+unsigned int lvl;
+
+hvm_cpuid(input & 0x, , NULL, NULL, NULL);
+if ( ((lvl ^ input) >> 16) || input > lvl )
+{
+if ( d->arch.x86_vendor == X86_VENDOR_AMD )
+{
+*eax = 0;
+*ebx = 0;
+*ecx = 0;
+*edx = 0;
+return;
+}
+if ( input >> 16 )
+hvm_cpuid(0, , NULL, NULL, NULL);
+input = lvl;
+}
+}
+
 if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) )
 return;
 
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -944,7 +944,40 @@ void pv_cpuid(struct cpu_user_regs *regs
 struct vcpu *curr = current;
 struct domain *currd = curr->domain;
 
-leaf = a = regs->eax;
+leaf = regs->eax;
+
+if ( leaf & 0x )
+{
+/*
+ * Requests beyond the highest supported leaf within a group return
+ * zero on AMD and the highest basic leaf output on others.
+ */
+if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
+domain_cpuid(currd, leaf & 0x, 0, , , , );
+else
+a = cpuid_eax(leaf & 0x);
+if ( ((a ^ leaf) >> 16) || leaf > a )
+{
+if ( currd->arch.x86_vendor == X86_VENDOR_AMD )
+{
+regs->eax = 0;
+regs->ebx = 0;
+regs->ecx = 0;
+regs->edx = 0;
+return;
+}
+if ( leaf >> 16 )
+{
+if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
+domain_cpuid(currd, 0, 0, , , , );
+else
+a = cpuid_eax(0);
+}
+leaf = a;
+}
+}
+
+a = regs->eax;
 b = regs->ebx;
 subleaf = c = regs->ecx;
 d = regs->edx;



x86: correct CPUID output for out of bounds input

Another place where we should try to behave like real hardware; see
the code comments.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3358,6 +3358,31 @@ void hvm_cpuid(unsigned int input, unsig
 if ( !edx )
 edx = 
 
+if ( input & 0x )
+{
+/*
+ * Requests beyond the highest supported leaf within a group return
+ * zero on AMD and the highest basic leaf output on others.
+ */
+unsigned int lvl;
+
+hvm_cpuid(input & 0x, , NULL, NULL, NULL);
+if ( ((lvl ^ input) >> 16) || input > lvl )
+{
+if ( d->arch.x86_vendor == X86_VENDOR_AMD )
+{
+*eax = 0;
+*ebx = 0;
+*ecx = 0;
+*edx = 0;
+return;
+}
+if ( input >> 16 )
+hvm_cpuid(0, , NULL, NULL, NULL);
+input = lvl;
+}
+}
+
 if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) )
 return;
 
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -944,7 +944,40 @@ void pv_cpuid(struct cpu_user_regs *regs
 struct vcpu *curr = current;
 struct domain *currd = curr->domain;
 
-leaf = a = regs->eax;
+leaf = regs->eax;
+
+if ( leaf & 0x )
+{
+/*
+ * Requests beyond the highest supported leaf within a group return
+ * zero on AMD and the highest basic leaf output on others.
+ */
+if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
+domain_cpuid(currd, leaf & 0x, 0, , , , );
+else
+a = cpuid_eax(leaf & 0x);
+if ( ((a ^ leaf) >> 16) || leaf > a )
+{
+if ( currd->arch.x86_vendor == X86_VENDOR_AMD )
+{
+regs->eax = 0;
+regs->ebx = 0;
+regs->ecx = 0;
+regs->edx = 0;
+return;
+}
+if ( leaf >> 16 )
+{
+if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
+domain_cpuid(currd, 0, 0, , , , );
+else
+a = cpuid_eax(0);
+}
+leaf = a;
+}
+}
+
+a = regs->eax;
 b = regs->ebx;
 subleaf = c = regs->ecx;
 d = regs->edx;
___
Xen-devel mailing list
Xen-devel@lists.xen.org