Patchwork kvm: x86: Clear {ebx,ecx,edx} from CPUID leaf 0x8000001a

login
register
mail settings
Submitter Jim Mattson
Date Feb. 8, 2019, 5:41 p.m.
Message ID <20190208174110.122127-1-jmattson@google.com>
Download mbox | patch
Permalink /patch/721829/
State New
Headers show

Comments

Jim Mattson - Feb. 8, 2019, 5:41 p.m.
According to volume 3 of the APM, appendix E, CPUID
Fn8000_001A_E[D,C,B]X are reserved. Since we have no way of knowing
what these fields will be used for, they should not be whitelisted.

Fixes: 24c82e576b78 ("KVM: Sanitize cpuid")
Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Peter Shier <pshier@google.com>
---
 arch/x86/kvm/cpuid.c | 1 +
 1 file changed, 1 insertion(+)
Christopherson, Sean J - Feb. 8, 2019, 6:19 p.m.
On Fri, Feb 08, 2019 at 09:41:10AM -0800, Jim Mattson wrote:
> According to volume 3 of the APM, appendix E, CPUID
> Fn8000_001A_E[D,C,B]X are reserved. Since we have no way of knowing
> what these fields will be used for, they should not be whitelisted.
> 
> Fixes: 24c82e576b78 ("KVM: Sanitize cpuid")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

> ---
>  arch/x86/kvm/cpuid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index bbffa6c54697..183f40cd3362 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -696,6 +696,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  		entry->ecx = entry->edx = 0;
>  		break;
>  	case 0x8000001a:
> +		entry->ebx = entry->ecx = entry->edx = 0;

Mostly of out of curiosity, but also a bit of devil's advocate, where
do we draw the line in terms of handling reserved fields/bits?  For
example, Fn8000_001A_EAX bits 31:3 appear to be reserved, should we
clear them as well?

I get that we want to find a balance between having to update KVM every
time a new bit is defined and exposing unknown data to VMs, just seems
like we're making a somewhat arbitrary decision as to how much we're
willing to trust hardware vendors to not do weird/stupid things.

Switching gears, leaf 0x9 should also get this treatment.

>  		break;
>  	case 0x8000001d:
>  		break;
> -- 
> 2.20.1.791.gb4d0f1c61a-goog
>
Jim Mattson - Feb. 8, 2019, 9:39 p.m.
On Fri, Feb 8, 2019 at 10:19 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:>> On Fri, Feb 08, 2019 at
09:41:10AM -0800, Jim Mattson wrote:> > According to volume 3 of the
APM, appendix E, CPUID> > Fn8000_001A_E[D,C,B]X are reserved. Since we
have no way of knowing> > what these fields will be used for, they
should not be whitelisted.> >> > Fixes: 24c82e576b78 ("KVM: Sanitize
cpuid")> > Signed-off-by: Jim Mattson <jmattson@google.com>> >
Reviewed-by: Peter Shier <pshier@google.com>>> Reviewed-by: Sean
Christopherson <sean.j.christopherson@intel.com>>> > ---> >
arch/x86/kvm/cpuid.c | 1 +> >  1 file changed, 1 insertion(+)> >> >
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c> > index
bbffa6c54697..183f40cd3362 100644> > --- a/arch/x86/kvm/cpuid.c> > +++
b/arch/x86/kvm/cpuid.c> > @@ -696,6 +696,7 @@ static inline int
__do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,> >
       entry->ecx = entry->edx = 0;> >               break;> >
case 0x8000001a:> > +             entry->ebx = entry->ecx = entry->edx
= 0;>> Mostly of out of curiosity, but also a bit of devil's advocate,
where> do we draw the line in terms of handling reserved fields/bits?
For> example, Fn8000_001A_EAX bits 31:3 appear to be reserved, should
we> clear them as well?I'm not sure what the answer is. I just saw the
way that leaf 0x80000019 was handled above, and it seemed obvious that
leaf 0x8000001a should get the same treatment.

> I get that we want to find a balance between having to update KVM every
> time a new bit is defined and exposing unknown data to VMs, just seems
> like we're making a somewhat arbitrary decision as to how much we're
> willing to trust hardware vendors to not do weird/stupid things.
>
> Switching gears, leaf 0x9 should also get this treatment.

I agree. I'll send that patch out in a moment.

> >               break;
> >       case 0x8000001d:
> >               break;
> > --
> > 2.20.1.791.gb4d0f1c61a-goog
> >

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index bbffa6c54697..183f40cd3362 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -696,6 +696,7 @@  static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		entry->ecx = entry->edx = 0;
 		break;
 	case 0x8000001a:
+		entry->ebx = entry->ecx = entry->edx = 0;
 		break;
 	case 0x8000001d:
 		break;