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: [email protected]
Signed-off-by: Sean Christopherson <[email protected]>
---
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
Sean Christopherson <[email protected]> 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: [email protected]
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> 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 <[email protected]>
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
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 <[email protected]>
>
> 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: [email protected]
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> 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 <[email protected]>
> >
> > 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: [email protected]
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > 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: [email protected]
>>> Signed-off-by: Sean Christopherson <[email protected]>
>>> ---
>>> 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: [email protected]
> > > Signed-off-by: Sean Christopherson <[email protected]>
> > > ---
> > > 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 <[email protected]> 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
On Fri, May 29, 2020 at 09:46:13AM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <[email protected]> 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!