* [PATCH] KVM: Sanitize cpuid
@ 2011-05-12 13:23 Avi Kivity
2011-05-18 7:19 ` Roedel, Joerg
0 siblings, 1 reply; 3+ messages in thread
From: Avi Kivity @ 2011-05-12 13:23 UTC (permalink / raw)
To: Marcelo Tosatti, kvm, Joerg Roedel
Instead of blacklisting known-unsupported cpuid leaves, whitelist known-
supported leaves. This is more conservative and prevents us from reporting
features we don't support. Also whitelist a few more leaves while at it.
Signed-off-by: Avi Kivity <avi@redhat.com>
---
Joerg, if you can review the AMD bits... I was somewhat dazed and confused
when I got there (but tried to continue).
arch/x86/kvm/x86.c | 37 +++++++++++++++++++++++++++++++++++--
1 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 77c9d867..78149d3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2283,6 +2283,13 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function,
entry->flags = 0;
}
+static bool supported_xcr0_bit(unsigned bit)
+{
+ u64 mask = ((u64)1 << bit);
+
+ return mask & (XSTATE_FP | XSTATE_SSE | XSTATE_YMM) & host_xcr0;
+}
+
#define F(x) bit(X86_FEATURE_##x)
static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
@@ -2393,6 +2400,8 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
}
break;
}
+ case 9:
+ break;
case 0xb: {
int i, level_type;
@@ -2414,7 +2423,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
for (i = 1; *nent < maxnent && i < 64; ++i) {
- if (entry[i].eax == 0)
+ if (entry[i].eax == 0 || !supported_xcr0_bit(i))
continue;
do_cpuid_1_ent(&entry[i], function, i);
entry[i].flags |=
@@ -2451,6 +2460,24 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
entry->ecx &= kvm_supported_word6_x86_features;
cpuid_mask(&entry->ecx, 6);
break;
+ case 0x80000008: {
+ u8 g_phys_as = entry->eax >> 16;
+ u8 virt_as = max(entry->eax >> 8, 48U);
+ u8 phys_as = entry->eax;
+
+ if (!g_phys_as)
+ g_phys_as = phys_as;
+ entry->eax = g_phys_as | (virt_as << 8);
+ entry->ebx = entry->edx = 0;
+ break;
+ }
+ case 0x80000019:
+ entry->ecx = entry->edx = 0;
+ break;
+ case 0x8000001a:
+ break;
+ case 0x8000001d:
+ break;
/*Add support for Centaur's CPUID instruction*/
case 0xC0000000:
/*Just support up to 0xC0000004 now*/
@@ -2460,10 +2487,16 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
entry->edx &= kvm_supported_word5_x86_features;
cpuid_mask(&entry->edx, 5);
break;
+ case 3: /* Processor serial number */
+ case 5: /* MONITOR/MWAIT */
+ case 6: /* Thermal management */
+ case 0xA: /* Architectural Performance Monitoring */
+ case 0x80000007: /* Advanced power management */
case 0xC0000002:
case 0xC0000003:
case 0xC0000004:
- /*Now nothing to do, reserved for the future*/
+ default:
+ entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
break;
}
--
1.7.4.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: Sanitize cpuid
2011-05-12 13:23 [PATCH] KVM: Sanitize cpuid Avi Kivity
@ 2011-05-18 7:19 ` Roedel, Joerg
2011-05-18 8:34 ` Avi Kivity
0 siblings, 1 reply; 3+ messages in thread
From: Roedel, Joerg @ 2011-05-18 7:19 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, kvm
On Thu, May 12, 2011 at 09:23:45AM -0400, Avi Kivity wrote:
> Instead of blacklisting known-unsupported cpuid leaves, whitelist known-
> supported leaves. This is more conservative and prevents us from reporting
> features we don't support. Also whitelist a few more leaves while at it.
Good improvement, some small annotations inline.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>
> Joerg, if you can review the AMD bits... I was somewhat dazed and confused
> when I got there (but tried to continue).
>
> arch/x86/kvm/x86.c | 37 +++++++++++++++++++++++++++++++++++--
> 1 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 77c9d867..78149d3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2283,6 +2283,13 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> entry->flags = 0;
> }
>
> +static bool supported_xcr0_bit(unsigned bit)
> +{
> + u64 mask = ((u64)1 << bit);
> +
> + return mask & (XSTATE_FP | XSTATE_SSE | XSTATE_YMM) & host_xcr0;
> +}
> +
> #define F(x) bit(X86_FEATURE_##x)
>
> static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> @@ -2393,6 +2400,8 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> }
> break;
> }
> + case 9:
> + break;
> case 0xb: {
> int i, level_type;
>
> @@ -2414,7 +2423,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>
> entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> for (i = 1; *nent < maxnent && i < 64; ++i) {
> - if (entry[i].eax == 0)
> + if (entry[i].eax == 0 || !supported_xcr0_bit(i))
> continue;
> do_cpuid_1_ent(&entry[i], function, i);
> entry[i].flags |=
> @@ -2451,6 +2460,24 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> entry->ecx &= kvm_supported_word6_x86_features;
> cpuid_mask(&entry->ecx, 6);
> break;
> + case 0x80000008: {
> + u8 g_phys_as = entry->eax >> 16;
> + u8 virt_as = max(entry->eax >> 8, 48U);
Shouldn't that be 'max((entry->eax >> 8) & 0xff, 48U)'? Seems safer when
the entry->eax contains a non-zero g_phys value.
> + u8 phys_as = entry->eax;
> +
> + if (!g_phys_as)
> + g_phys_as = phys_as;
> + entry->eax = g_phys_as | (virt_as << 8);
It is optional, but since we support nesting we can also encode
g_phys_as in bits 23:16.
> + entry->ebx = entry->edx = 0;
> + break;
> + }
> + case 0x80000019:
> + entry->ecx = entry->edx = 0;
> + break;
> + case 0x8000001a:
> + break;
> + case 0x8000001d:
> + break;
> /*Add support for Centaur's CPUID instruction*/
> case 0xC0000000:
> /*Just support up to 0xC0000004 now*/
> @@ -2460,10 +2487,16 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> entry->edx &= kvm_supported_word5_x86_features;
> cpuid_mask(&entry->edx, 5);
> break;
> + case 3: /* Processor serial number */
> + case 5: /* MONITOR/MWAIT */
> + case 6: /* Thermal management */
> + case 0xA: /* Architectural Performance Monitoring */
> + case 0x80000007: /* Advanced power management */
> case 0xC0000002:
> case 0xC0000003:
> case 0xC0000004:
> - /*Now nothing to do, reserved for the future*/
> + default:
> + entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> break;
> }
>
> --
> 1.7.4.3
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: Sanitize cpuid
2011-05-18 7:19 ` Roedel, Joerg
@ 2011-05-18 8:34 ` Avi Kivity
0 siblings, 0 replies; 3+ messages in thread
From: Avi Kivity @ 2011-05-18 8:34 UTC (permalink / raw)
To: Roedel, Joerg; +Cc: Marcelo Tosatti, kvm
On 05/18/2011 10:19 AM, Roedel, Joerg wrote:
> > break;
> > + case 0x80000008: {
> > + u8 g_phys_as = entry->eax>> 16;
> > + u8 virt_as = max(entry->eax>> 8, 48U);
>
> Shouldn't that be 'max((entry->eax>> 8)& 0xff, 48U)'? Seems safer when
> the entry->eax contains a non-zero g_phys value.
Yes, this is a bug. I originally had 'u8 virt_as = entry->eax >> 8',
relying on the cast to u8, but missed it when updating. Moral: don't be
subtle.
> > + u8 phys_as = entry->eax;
> > +
> > + if (!g_phys_as)
> > + g_phys_as = phys_as;
> > + entry->eax = g_phys_as | (virt_as<< 8);
>
> It is optional, but since we support nesting we can also encode
> g_phys_as in bits 23:16.
I'm relying on a zero value in 23:16 indicating you should use 7:0.
Thanks for the review, will post an updated patch.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-05-18 8:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-12 13:23 [PATCH] KVM: Sanitize cpuid Avi Kivity
2011-05-18 7:19 ` Roedel, Joerg
2011-05-18 8:34 ` Avi Kivity
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.