[PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Sean Christopherson]

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;
 
-- 
2.26.0

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Vitaly Kuznetsov]

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. 

-- 
Vitaly

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Paolo Bonzini]

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

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Sean Christopherson]

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.

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Paolo Bonzini]

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

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Sean Christopherson]

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.

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Paolo Bonzini]

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

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Sean Christopherson]

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.

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Paolo Bonzini]

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

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Sean Christopherson]

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.

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Sean Christopherson]

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? 

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Vitaly Kuznetsov]

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?

-- 
Vitaly

Re: [PATCH] KVM: x86: Initialize tdp_level during vCPU creation

[Sean Christopherson]

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!