| Message ID | 20200527085400.23759-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Initialize tdp_level during vCPU creation | expand |
Sean Christopherson <sean.j.christopherson@intel.com> writes: > Initialize vcpu->arch.tdp_level during vCPU creation to avoid consuming > garbage if userspace calls KVM_RUN without first calling KVM_SET_CPUID. > > Fixes: e93fd3b3e89e9 ("KVM: x86/mmu: Capture TDP level when updating CPUID") > Reported-by: syzbot+904752567107eefb728c@syzkaller.appspotmail.com > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/x86.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index b226fb8abe41b..01a6304056197 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -9414,6 +9414,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) > fx_init(vcpu); > > vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); > + vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu); > > vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> Looking at kvm_update_cpuid() I was thinking if it would make sense to duplicate the "/* Note, maxphyaddr must be updated before tdp_level. */" comment here (it seems to be a vmx-only thing btw), drop it from kvm_update_cpuid() or move cpuid_query_maxphyaddr() to get_tdp_level() but didn't come to a conclusive answer.
On 27/05/20 12:03, Vitaly Kuznetsov wrote: >> >> vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); >> + vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu); >> >> vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > Looking at kvm_update_cpuid() I was thinking if it would make sense to > duplicate the "/* Note, maxphyaddr must be updated before tdp_level. */" > comment here (it seems to be a vmx-only thing btw), drop it from > kvm_update_cpuid() or move cpuid_query_maxphyaddr() to get_tdp_level() > but didn't come to a conclusive answer. Yeah, it makes sense to at least add the comment here too. Paolo
On 27/05/20 10:54, Sean Christopherson wrote: > Initialize vcpu->arch.tdp_level during vCPU creation to avoid consuming > garbage if userspace calls KVM_RUN without first calling KVM_SET_CPUID. > > Fixes: e93fd3b3e89e9 ("KVM: x86/mmu: Capture TDP level when updating CPUID") > Reported-by: syzbot+904752567107eefb728c@syzkaller.appspotmail.com > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/x86.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index b226fb8abe41b..01a6304056197 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -9414,6 +9414,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) > fx_init(vcpu); > > vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); > + vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu); > > vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; Queued, it is probably a good idea to add a selftests testcase for this (it's even okay if it doesn't use the whole selftests infrastructure and invokes KVM_CREATE_VM/KVM_CREATE_VCPU/KVM_RUN manually). Paolo
On Wed, May 27, 2020 at 06:15:27PM +0200, Paolo Bonzini wrote: > On 27/05/20 12:03, Vitaly Kuznetsov wrote: > >> > >> vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); > >> + vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu); > >> > >> vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > > > Looking at kvm_update_cpuid() I was thinking if it would make sense to > > duplicate the "/* Note, maxphyaddr must be updated before tdp_level. */" > > comment here (it seems to be a vmx-only thing btw), drop it from > > kvm_update_cpuid() or move cpuid_query_maxphyaddr() to get_tdp_level() > > but didn't come to a conclusive answer. > > Yeah, it makes sense to at least add the comment here too. Hmm, one option would be to make .get_tdp_level() pure function by passing in vcpu->arch.maxphyaddr. That should make the comment redundant. I don't love bleeding VMX's implementation into the prototype, but that ship has kinda already sailed. Side topic, cpuid_query_maxphyaddr() should be unexported, the RTIT usage can and should use vcpu->arch.maxphyaddr. I'll send a patch for that.
On Wed, May 27, 2020 at 06:17:57PM +0200, Paolo Bonzini wrote: > On 27/05/20 10:54, Sean Christopherson wrote: > > Initialize vcpu->arch.tdp_level during vCPU creation to avoid consuming > > garbage if userspace calls KVM_RUN without first calling KVM_SET_CPUID. > > > > Fixes: e93fd3b3e89e9 ("KVM: x86/mmu: Capture TDP level when updating CPUID") > > Reported-by: syzbot+904752567107eefb728c@syzkaller.appspotmail.com > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kvm/x86.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index b226fb8abe41b..01a6304056197 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -9414,6 +9414,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) > > fx_init(vcpu); > > > > vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); > > + vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu); > > > > vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; > > Queued, it is probably a good idea to add a selftests testcase for this > (it's even okay if it doesn't use the whole selftests infrastructure and > invokes KVM_CREATE_VM/KVM_CREATE_VCPU/KVM_RUN manually). Ya. syzbot is hitting a #GP due to NULL pointer during debugfs on the exact same sequence. I haven't been able to reproduce that one (have yet to try syzbot's exact config), but it's another example of a "dumb" test hitting meaningful bugs.
On 27/05/20 18:23, Sean Christopherson wrote: > Hmm, one option would be to make .get_tdp_level() pure function by passing > in vcpu->arch.maxphyaddr. That should make the comment redundant. I don't > love bleeding VMX's implementation into the prototype, but that ship has > kinda already sailed. Well, it's not bleeding the implementation that much, guest MAXPHYADDR is pretty much the only reason why it's a function and not a constant. Another possibility BTW is to make the callback get_max_tdp_level and make get_tdp_level a function in mmu.c. Paolo
On 27/05/20 18:29, Sean Christopherson wrote: > On Wed, May 27, 2020 at 06:17:57PM +0200, Paolo Bonzini wrote: >> On 27/05/20 10:54, Sean Christopherson wrote: >>> Initialize vcpu->arch.tdp_level during vCPU creation to avoid consuming >>> garbage if userspace calls KVM_RUN without first calling KVM_SET_CPUID. >>> >>> Fixes: e93fd3b3e89e9 ("KVM: x86/mmu: Capture TDP level when updating CPUID") >>> Reported-by: syzbot+904752567107eefb728c@syzkaller.appspotmail.com >>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> >>> --- >>> arch/x86/kvm/x86.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index b226fb8abe41b..01a6304056197 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -9414,6 +9414,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) >>> fx_init(vcpu); >>> >>> vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); >>> + vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu); >>> >>> vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; >> >> Queued, it is probably a good idea to add a selftests testcase for this >> (it's even okay if it doesn't use the whole selftests infrastructure and >> invokes KVM_CREATE_VM/KVM_CREATE_VCPU/KVM_RUN manually). > > Ya. syzbot is hitting a #GP due to NULL pointer during debugfs on the exact > same sequence. I haven't been able to reproduce that one (have yet to try > syzbot's exact config), but it's another example of a "dumb" test hitting > meaningful bugs. Saw that, it's mine. :) Paolo
On Wed, May 27, 2020 at 06:56:19PM +0200, Paolo Bonzini wrote: > On 27/05/20 18:29, Sean Christopherson wrote: > > Ya. syzbot is hitting a #GP due to NULL pointer during debugfs on the exact > > same sequence. I haven't been able to reproduce that one (have yet to try > > syzbot's exact config), but it's another example of a "dumb" test hitting > > meaningful bugs. > > Saw that, it's mine. :) All yours. I as hoping it would be easily reproducible and fixable while I was looking at the MMU BUG(), but that didn't happen.
On Wed, May 27, 2020 at 06:56:02PM +0200, Paolo Bonzini wrote: > On 27/05/20 18:23, Sean Christopherson wrote: > > Hmm, one option would be to make .get_tdp_level() pure function by passing > > in vcpu->arch.maxphyaddr. That should make the comment redundant. I don't > > love bleeding VMX's implementation into the prototype, but that ship has > > kinda already sailed. > > Well, it's not bleeding the implementation that much, guest MAXPHYADDR > is pretty much the only reason why it's a function and not a constant. > > Another possibility BTW is to make the callback get_max_tdp_level and > make get_tdp_level a function in mmu.c. I like that idea. We could even avoid get_max_tdp_level() by passing that info into kvm_configure_mmu(), though that may not actually be cleaner. I'll play around with it.
On Wed, May 27, 2020 at 09:29:33AM -0700, Sean Christopherson wrote: > On Wed, May 27, 2020 at 06:17:57PM +0200, Paolo Bonzini wrote: > > On 27/05/20 10:54, Sean Christopherson wrote: > > > Initialize vcpu->arch.tdp_level during vCPU creation to avoid consuming > > > garbage if userspace calls KVM_RUN without first calling KVM_SET_CPUID. > > > > > > Fixes: e93fd3b3e89e9 ("KVM: x86/mmu: Capture TDP level when updating CPUID") > > > Reported-by: syzbot+904752567107eefb728c@syzkaller.appspotmail.com > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > --- > > > arch/x86/kvm/x86.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index b226fb8abe41b..01a6304056197 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -9414,6 +9414,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) > > > fx_init(vcpu); > > > > > > vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); > > > + vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu); > > > > > > vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT; > > > > Queued, it is probably a good idea to add a selftests testcase for this > > (it's even okay if it doesn't use the whole selftests infrastructure and > > invokes KVM_CREATE_VM/KVM_CREATE_VCPU/KVM_RUN manually). > > Ya. syzbot is hitting a #GP due to NULL pointer during debugfs on the exact > same sequence. I haven't been able to reproduce that one (have yet to try > syzbot's exact config), but it's another example of a "dumb" test hitting > meaningful bugs. So we already have a selftest for this scenario, test_zero_memory_regions() in set_memory_region_test.c. Embarrassingly, I wrote the darn thing, I'm just really neglectful when it comes to running selftests. I'll looking into writing a script to run all selftests with a single command, unless someone already has one laying around?
Sean Christopherson <sean.j.christopherson@intel.com> writes: > I'll looking into writing a script to run all selftests with a single > command, unless someone already has one laying around? Is 'make run_tests' in tools/testing/selftests/kvm/ what you're looking for?
On Fri, May 29, 2020 at 09:46:13AM +0200, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > > > I'll looking into writing a script to run all selftests with a single > > command, unless someone already has one laying around? > > Is 'make run_tests' in tools/testing/selftests/kvm/ what you're looking > for? *sigh* Yes. Thanks!
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index b226fb8abe41b..01a6304056197 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -9414,6 +9414,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) fx_init(vcpu); vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); + vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu); vcpu->arch.pat = MSR_IA32_CR_PAT_DEFAULT;
Initialize vcpu->arch.tdp_level during vCPU creation to avoid consuming garbage if userspace calls KVM_RUN without first calling KVM_SET_CPUID. Fixes: e93fd3b3e89e9 ("KVM: x86/mmu: Capture TDP level when updating CPUID") Reported-by: syzbot+904752567107eefb728c@syzkaller.appspotmail.com Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/x86.c | 1 + 1 file changed, 1 insertion(+)