[PATCH 0/2] KVM: x86: Partially allow KVM_SET_CPUID{,2} follow-up

[PATCH 0/2] KVM: x86: Partially allow KVM_SET_CPUID{,2} follow-up

[Vitaly Kuznetsov]

"[PATCH v3 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN
 for CPU hotplug" got merged but v4 had some important improvements:
- Fix for SGX enabled CPUs
- Also check .flags in kvm_cpuid_check_equal().
Sending these out separately.

Vitaly Kuznetsov (2):
  KVM: x86: Move CPUID.(EAX=0x12,ECX=1) mangling to
    __kvm_update_cpuid_runtime()
  KVM: x86: Use memcmp in kvm_cpuid_check_equal()

 arch/x86/kvm/cpuid.c | 67 +++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH 1/2] KVM: x86: Move CPUID.(EAX=0x12,ECX=1) mangling to __kvm_update_cpuid_runtime()

[Vitaly Kuznetsov]

Full equality check of CPUID data on update (kvm_cpuid_check_equal()) may
fail for SGX enabled CPUs as CPUID.(EAX=0x12,ECX=1) is currently being
mangled in kvm_vcpu_after_set_cpuid(). Move it to
__kvm_update_cpuid_runtime() and split off cpuid_get_supported_xcr0()
helper  as 'vcpu->arch.guest_supported_xcr0' update needs (logically)
to stay in kvm_vcpu_after_set_cpuid().

Cc: stable@vger.kernel.org
Fixes: feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/cpuid.c | 54 +++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 3902c28fb6cb..89d7822a8f5b 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -196,10 +196,26 @@ void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
 		vcpu->arch.pv_cpuid.features = best->eax;
 }
 
+/*
+ * Calculate guest's supported XCR0 taking into account guest CPUID data and
+ * supported_xcr0 (comprised of host configuration and KVM_SUPPORTED_XCR0).
+ */
+static u64 cpuid_get_supported_xcr0(struct kvm_cpuid_entry2 *entries, int nent)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = cpuid_entry2_find(entries, nent, 0xd, 0);
+	if (!best)
+		return 0;
+
+	return (best->eax | ((u64)best->edx << 32)) & supported_xcr0;
+}
+
 static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
 				       int nent)
 {
 	struct kvm_cpuid_entry2 *best;
+	u64 guest_supported_xcr0 = cpuid_get_supported_xcr0(entries, nent);
 
 	best = cpuid_entry2_find(entries, nent, 1, 0);
 	if (best) {
@@ -238,6 +254,21 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
 					   vcpu->arch.ia32_misc_enable_msr &
 					   MSR_IA32_MISC_ENABLE_MWAIT);
 	}
+
+	/*
+	 * Bits 127:0 of the allowed SECS.ATTRIBUTES (CPUID.0x12.0x1) enumerate
+	 * the supported XSAVE Feature Request Mask (XFRM), i.e. the enclave's
+	 * requested XCR0 value.  The enclave's XFRM must be a subset of XCRO
+	 * at the time of EENTER, thus adjust the allowed XFRM by the guest's
+	 * supported XCR0.  Similar to XCR0 handling, FP and SSE are forced to
+	 * '1' even on CPUs that don't support XSAVE.
+	 */
+	best = cpuid_entry2_find(entries, nent, 0x12, 0x1);
+	if (best) {
+		best->ecx &= guest_supported_xcr0 & 0xffffffff;
+		best->edx &= guest_supported_xcr0 >> 32;
+		best->ecx |= XFEATURE_MASK_FPSSE;
+	}
 }
 
 void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
@@ -261,27 +292,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 		kvm_apic_set_version(vcpu);
 	}
 
-	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
-	if (!best)
-		vcpu->arch.guest_supported_xcr0 = 0;
-	else
-		vcpu->arch.guest_supported_xcr0 =
-			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
-
-	/*
-	 * Bits 127:0 of the allowed SECS.ATTRIBUTES (CPUID.0x12.0x1) enumerate
-	 * the supported XSAVE Feature Request Mask (XFRM), i.e. the enclave's
-	 * requested XCR0 value.  The enclave's XFRM must be a subset of XCRO
-	 * at the time of EENTER, thus adjust the allowed XFRM by the guest's
-	 * supported XCR0.  Similar to XCR0 handling, FP and SSE are forced to
-	 * '1' even on CPUs that don't support XSAVE.
-	 */
-	best = kvm_find_cpuid_entry(vcpu, 0x12, 0x1);
-	if (best) {
-		best->ecx &= vcpu->arch.guest_supported_xcr0 & 0xffffffff;
-		best->edx &= vcpu->arch.guest_supported_xcr0 >> 32;
-		best->ecx |= XFEATURE_MASK_FPSSE;
-	}
+	vcpu->arch.guest_supported_xcr0 =
+		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
 
 	kvm_update_pv_runtime(vcpu);
 
-- 
2.34.1

[PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Vitaly Kuznetsov]

kvm_cpuid_check_equal() should also check .flags equality but instead
of adding it to the existing check, just switch to using memcmp() for
the whole 'struct kvm_cpuid_entry2'.

When .flags are not checked, kvm_cpuid_check_equal() may allow an update
which it shouldn't but kvm_set_cpuid() does not actually update anything
and just returns success.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/cpuid.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 89d7822a8f5b..7dd9c8f4f46e 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -123,20 +123,11 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu,
 static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 				 int nent)
 {
-	struct kvm_cpuid_entry2 *orig;
-	int i;
-
 	if (nent != vcpu->arch.cpuid_nent)
 		return -EINVAL;
 
-	for (i = 0; i < nent; i++) {
-		orig = &vcpu->arch.cpuid_entries[i];
-		if (e2[i].function != orig->function ||
-		    e2[i].index != orig->index ||
-		    e2[i].eax != orig->eax || e2[i].ebx != orig->ebx ||
-		    e2[i].ecx != orig->ecx || e2[i].edx != orig->edx)
-			return -EINVAL;
-	}
+	if (memcmp(e2, vcpu->arch.cpuid_entries, nent * sizeof(*e2)))
+		return -EINVAL;
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Paolo Bonzini]

On 1/24/22 11:36, Vitaly Kuznetsov wrote:
> kvm_cpuid_check_equal() should also check .flags equality but instead
> of adding it to the existing check, just switch to using memcmp() for
> the whole 'struct kvm_cpuid_entry2'.
> 
> When .flags are not checked, kvm_cpuid_check_equal() may allow an update
> which it shouldn't but kvm_set_cpuid() does not actually update anything
> and just returns success.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>   arch/x86/kvm/cpuid.c | 13 ++-----------
>   1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 89d7822a8f5b..7dd9c8f4f46e 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -123,20 +123,11 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu,
>   static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>   				 int nent)
>   {
> -	struct kvm_cpuid_entry2 *orig;
> -	int i;
> -
>   	if (nent != vcpu->arch.cpuid_nent)
>   		return -EINVAL;
>   
> -	for (i = 0; i < nent; i++) {
> -		orig = &vcpu->arch.cpuid_entries[i];
> -		if (e2[i].function != orig->function ||
> -		    e2[i].index != orig->index ||
> -		    e2[i].eax != orig->eax || e2[i].ebx != orig->ebx ||
> -		    e2[i].ecx != orig->ecx || e2[i].edx != orig->edx)
> -			return -EINVAL;
> -	}
> +	if (memcmp(e2, vcpu->arch.cpuid_entries, nent * sizeof(*e2)))
> +		return -EINVAL;

Hmm, not sure about that due to the padding in struct kvm_cpuid_entry2. 
  It might break userspace that isn't too careful about zeroing it.

Queued patch 1 though.

Paolo

Re: [PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 1/24/22 11:36, Vitaly Kuznetsov wrote:
>> kvm_cpuid_check_equal() should also check .flags equality but instead
>> of adding it to the existing check, just switch to using memcmp() for
>> the whole 'struct kvm_cpuid_entry2'.
>> 
>> When .flags are not checked, kvm_cpuid_check_equal() may allow an update
>> which it shouldn't but kvm_set_cpuid() does not actually update anything
>> and just returns success.
>> 
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 13 ++-----------
>>   1 file changed, 2 insertions(+), 11 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 89d7822a8f5b..7dd9c8f4f46e 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -123,20 +123,11 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu,
>>   static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>>   				 int nent)
>>   {
>> -	struct kvm_cpuid_entry2 *orig;
>> -	int i;
>> -
>>   	if (nent != vcpu->arch.cpuid_nent)
>>   		return -EINVAL;
>>   
>> -	for (i = 0; i < nent; i++) {
>> -		orig = &vcpu->arch.cpuid_entries[i];
>> -		if (e2[i].function != orig->function ||
>> -		    e2[i].index != orig->index ||
>> -		    e2[i].eax != orig->eax || e2[i].ebx != orig->ebx ||
>> -		    e2[i].ecx != orig->ecx || e2[i].edx != orig->edx)
>> -			return -EINVAL;
>> -	}
>> +	if (memcmp(e2, vcpu->arch.cpuid_entries, nent * sizeof(*e2)))
>> +		return -EINVAL;
>
> Hmm, not sure about that due to the padding in struct kvm_cpuid_entry2. 
>   It might break userspace that isn't too careful about zeroing it.

FWIW, QEMU zeroes the whole thing before setting individual CPUID
entries. Legacy KVM_SET_CPUID call is also not afffected as it copies
entries to a newly allocated "struct kvm_cpuid_entry2[]" and explicitly
zeroes padding.

Do we need to at least add a check for ".flags"?

>
> Queued patch 1 though.
>
> Paolo
>

-- 
Vitaly

Re: [PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Sean Christopherson]

On Mon, Jan 24, 2022, Vitaly Kuznetsov wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> +	if (memcmp(e2, vcpu->arch.cpuid_entries, nent * sizeof(*e2)))
> >> +		return -EINVAL;
> >
> > Hmm, not sure about that due to the padding in struct kvm_cpuid_entry2. 
> >   It might break userspace that isn't too careful about zeroing it.

Given that we already are fully committed to potentially breaking userspace by
disallowing KVM_SET_CPUID{2} after KVM_RUN, we might as well get greedy.

> FWIW, QEMU zeroes the whole thing before setting individual CPUID
> entries. Legacy KVM_SET_CPUID call is also not afffected as it copies
> entries to a newly allocated "struct kvm_cpuid_entry2[]" and explicitly
> zeroes padding.
> 
> Do we need to at least add a check for ".flags"?

Yes.

Re: [PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Paolo Bonzini]

On 1/24/22 17:52, Sean Christopherson wrote:
> On Mon, Jan 24, 2022, Vitaly Kuznetsov wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>> +	if (memcmp(e2, vcpu->arch.cpuid_entries, nent * sizeof(*e2)))
>>>> +		return -EINVAL;
>>>
>>> Hmm, not sure about that due to the padding in struct kvm_cpuid_entry2.
>>>    It might break userspace that isn't too careful about zeroing it.
> 
> Given that we already are fully committed to potentially breaking userspace by
> disallowing KVM_SET_CPUID{2} after KVM_RUN, we might as well get greedy.

Hmm, I thought this series was because we were _not_ fully committed. :)

>> FWIW, QEMU zeroes the whole thing before setting individual CPUID
>> entries. Legacy KVM_SET_CPUID call is also not afffected as it copies
>> entries to a newly allocated "struct kvm_cpuid_entry2[]" and explicitly
>> zeroes padding.
>>
>> Do we need to at least add a check for ".flags"?
> 
> Yes.

Yes, we do.  Alternatively, we can replace memdup with a copy in the 
style of KVM_CPUID.

Paolo

Re: [PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Sean Christopherson]

On Mon, Jan 24, 2022, Paolo Bonzini wrote:
> On 1/24/22 17:52, Sean Christopherson wrote:
> > On Mon, Jan 24, 2022, Vitaly Kuznetsov wrote:
> > > Paolo Bonzini <pbonzini@redhat.com> writes:
> > > > > +	if (memcmp(e2, vcpu->arch.cpuid_entries, nent * sizeof(*e2)))
> > > > > +		return -EINVAL;
> > > > 
> > > > Hmm, not sure about that due to the padding in struct kvm_cpuid_entry2.
> > > >    It might break userspace that isn't too careful about zeroing it.
> > 
> > Given that we already are fully committed to potentially breaking userspace by
> > disallowing KVM_SET_CPUID{2} after KVM_RUN, we might as well get greedy.
> 
> Hmm, I thought this series was because we were _not_ fully committed. :)

We're fully committed in the sense that we know disallowing KVM_SET_CPUID2 after
KVM_RUN broke at least one use case, and instead of reverting we are doubling down
and adding more KVM code/complexity to grandfather in that one use case.  There's
no guarantee that there aren't other use cases that will break, but haven't been
reported simply because their users haven't yet moved to a 5.16 kernel.

Re: [PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Joe Perches]

On Mon, 2022-01-24 at 11:36 +0100, Vitaly Kuznetsov wrote:
> kvm_cpuid_check_equal() should also check .flags equality but instead
> of adding it to the existing check, just switch to using memcmp() for
> the whole 'struct kvm_cpuid_entry2'.

Is the struct padding guaranteed to be identical ?

> 
> When .flags are not checked, kvm_cpuid_check_equal() may allow an update
> which it shouldn't but kvm_set_cpuid() does not actually update anything
> and just returns success.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/cpuid.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 89d7822a8f5b..7dd9c8f4f46e 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -123,20 +123,11 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu,
>  static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>  				 int nent)
>  {
> -	struct kvm_cpuid_entry2 *orig;
> -	int i;
> -
>  	if (nent != vcpu->arch.cpuid_nent)
>  		return -EINVAL;
>  
> -	for (i = 0; i < nent; i++) {
> -		orig = &vcpu->arch.cpuid_entries[i];
> -		if (e2[i].function != orig->function ||
> -		    e2[i].index != orig->index ||
> -		    e2[i].eax != orig->eax || e2[i].ebx != orig->ebx ||
> -		    e2[i].ecx != orig->ecx || e2[i].edx != orig->edx)
> -			return -EINVAL;
> -	}
> +	if (memcmp(e2, vcpu->arch.cpuid_entries, nent * sizeof(*e2)))
> +		return -EINVAL;
>  
>  	return 0;
>  }

Re: [PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Vitaly Kuznetsov]

Joe Perches <joe@perches.com> writes:

> On Mon, 2022-01-24 at 11:36 +0100, Vitaly Kuznetsov wrote:
>> kvm_cpuid_check_equal() should also check .flags equality but instead
>> of adding it to the existing check, just switch to using memcmp() for
>> the whole 'struct kvm_cpuid_entry2'.
>
> Is the struct padding guaranteed to be identical ?
>

Well, yes (or we're all doomeed):
- 'struct kvm_cpuid_entry2' is part of KVM userspace ABI, it is supposed
to be stable.
- Here we compare structs which come from the same userspace during one
session (vCPU fd stays open), I can't imagine how structure layout can
change on-the-fly.

-- 
Vitaly

Re: [PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Sean Christopherson]

On Wed, Jan 26, 2022, Vitaly Kuznetsov wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > On Mon, 2022-01-24 at 11:36 +0100, Vitaly Kuznetsov wrote:
> >> kvm_cpuid_check_equal() should also check .flags equality but instead
> >> of adding it to the existing check, just switch to using memcmp() for
> >> the whole 'struct kvm_cpuid_entry2'.
> >
> > Is the struct padding guaranteed to be identical ?
> >
> 
> Well, yes (or we're all doomeed):
> - 'struct kvm_cpuid_entry2' is part of KVM userspace ABI, it is supposed
> to be stable.
> - Here we compare structs which come from the same userspace during one
> session (vCPU fd stays open), I can't imagine how structure layout can
> change on-the-fly.

I'm pretty sure Joe was asking if the contents of the padding field would be
identical, i.e. if KVM can guarnatee there won't be false positives on mismatches,
which is the same reason Paolo passed on this patch.  Though I still think we
should roll the dice :-)

Re: [PATCH 2/2] KVM: x86: Use memcmp in kvm_cpuid_check_equal()

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Jan 26, 2022, Vitaly Kuznetsov wrote:
>> Joe Perches <joe@perches.com> writes:
>> 
>> > On Mon, 2022-01-24 at 11:36 +0100, Vitaly Kuznetsov wrote:
>> >> kvm_cpuid_check_equal() should also check .flags equality but instead
>> >> of adding it to the existing check, just switch to using memcmp() for
>> >> the whole 'struct kvm_cpuid_entry2'.
>> >
>> > Is the struct padding guaranteed to be identical ?
>> >
>> 
>> Well, yes (or we're all doomeed):
>> - 'struct kvm_cpuid_entry2' is part of KVM userspace ABI, it is supposed
>> to be stable.
>> - Here we compare structs which come from the same userspace during one
>> session (vCPU fd stays open), I can't imagine how structure layout can
>> change on-the-fly.
>
> I'm pretty sure Joe was asking if the contents of the padding field would be
> identical, i.e. if KVM can guarnatee there won't be false positives on
> mismatches,

Ah, sorry, I thought about structure layout. Generally, there's no
guarantee the content of the padding will be the same in the
KVM_SET_CPUID2 case as we vmemdup_user() what userspace VMM gives us.

> which is the same reason Paolo passed on this patch.  Though I still think we
> should roll the dice :-)

Well, so far we've only identified CPU (re-)hotplug by reusing an
existing vCPU fd as a broken use-case and it may happen that QEMU is the
only VMM which does that (and memcmp() approach works for it) ... but
who know what's out there)

-- 
Vitaly