diff mbox series

[v4,1/4] KVM: x86: Fix a spurious -E2BIG in KVM_GET_EMULATED_CPUID

Message ID 20210408114303.30310-2-eesposit@redhat.com
State Superseded
Headers show
Series [v4,1/4] KVM: x86: Fix a spurious -E2BIG in KVM_GET_EMULATED_CPUID | expand

Commit Message

Emanuele Giuseppe Esposito April 8, 2021, 11:43 a.m. UTC
When retrieving emulated CPUID entries, check for an insufficient array
size if and only if KVM is actually inserting an entry.
If userspace has a priori knowledge of the exact array size,
KVM_GET_EMULATED_CPUID will incorrectly fail due to effectively requiring
an extra, unused entry.

Fixes: 433f4ba19041 ("KVM: x86: fix out-of-bounds write in KVM_GET_EMULATED_CPUID (CVE-2019-19332)")
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 arch/x86/kvm/cpuid.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Comments

Sean Christopherson April 8, 2021, 8:29 p.m. UTC | #1
On Thu, Apr 08, 2021, Emanuele Giuseppe Esposito wrote:
> When retrieving emulated CPUID entries, check for an insufficient array
> size if and only if KVM is actually inserting an entry.
> If userspace has a priori knowledge of the exact array size,
> KVM_GET_EMULATED_CPUID will incorrectly fail due to effectively requiring
> an extra, unused entry.
> 
> Fixes: 433f4ba19041 ("KVM: x86: fix out-of-bounds write in KVM_GET_EMULATED_CPUID (CVE-2019-19332)")
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  arch/x86/kvm/cpuid.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 6bd2f8b830e4..d30194081892 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -567,34 +567,33 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array,
>  
>  static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
>  {
> -	struct kvm_cpuid_entry2 *entry;
> -
> -	if (array->nent >= array->maxnent)
> -		return -E2BIG;
> +	struct kvm_cpuid_entry2 entry;
>  
> -	entry = &array->entries[array->nent];
> -	entry->function = func;
> -	entry->index = 0;
> -	entry->flags = 0;
> +	memset(&entry, 0, sizeof(entry));
>  
>  	switch (func) {
>  	case 0:
> -		entry->eax = 7;
> -		++array->nent;
> +		entry.eax = 7;
>  		break;
>  	case 1:
> -		entry->ecx = F(MOVBE);
> -		++array->nent;
> +		entry.ecx = F(MOVBE);
>  		break;
>  	case 7:
> -		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> -		entry->eax = 0;
> -		entry->ecx = F(RDPID);
> -		++array->nent;
> -	default:
> +		entry.flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> +		entry.ecx = F(RDPID);
>  		break;
> +	default:
> +		goto out;
>  	}
>  
> +	/* This check is performed only when func is valid */

Sorry to keep nitpicking and bikeshedding.  Funcs aren't really "invalid", KVM
just doesn't have any features it emulates in other leafs.  Maybe be more literal
in describing what triggers the check?

	/* Check the array capacity iff the entry is being copied over. */

Not a sticking point, so either way:

Reviewed-by: Sean Christopherson <seanjc@google.com>

> +	if (array->nent >= array->maxnent)
> +		return -E2BIG;
> +
> +	entry.function = func;
> +	memcpy(&array->entries[array->nent++], &entry, sizeof(entry));
> +
> +out:
>  	return 0;
>  }
>  
> -- 
> 2.30.2
>
Emanuele Giuseppe Esposito April 9, 2021, 12:34 p.m. UTC | #2
On 08/04/2021 22:29, Sean Christopherson wrote:
> On Thu, Apr 08, 2021, Emanuele Giuseppe Esposito wrote:

>> When retrieving emulated CPUID entries, check for an insufficient array

>> size if and only if KVM is actually inserting an entry.

>> If userspace has a priori knowledge of the exact array size,

>> KVM_GET_EMULATED_CPUID will incorrectly fail due to effectively requiring

>> an extra, unused entry.

>>

>> Fixes: 433f4ba19041 ("KVM: x86: fix out-of-bounds write in KVM_GET_EMULATED_CPUID (CVE-2019-19332)")

>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

>> ---

>>   arch/x86/kvm/cpuid.c | 33 ++++++++++++++++-----------------

>>   1 file changed, 16 insertions(+), 17 deletions(-)

>>

>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c

>> index 6bd2f8b830e4..d30194081892 100644

>> --- a/arch/x86/kvm/cpuid.c

>> +++ b/arch/x86/kvm/cpuid.c

>> @@ -567,34 +567,33 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array,

>>   

>>   static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)

>>   {

>> -	struct kvm_cpuid_entry2 *entry;

>> -

>> -	if (array->nent >= array->maxnent)

>> -		return -E2BIG;

>> +	struct kvm_cpuid_entry2 entry;

>>   

>> -	entry = &array->entries[array->nent];

>> -	entry->function = func;

>> -	entry->index = 0;

>> -	entry->flags = 0;

>> +	memset(&entry, 0, sizeof(entry));

>>   

>>   	switch (func) {

>>   	case 0:

>> -		entry->eax = 7;

>> -		++array->nent;

>> +		entry.eax = 7;

>>   		break;

>>   	case 1:

>> -		entry->ecx = F(MOVBE);

>> -		++array->nent;

>> +		entry.ecx = F(MOVBE);

>>   		break;

>>   	case 7:

>> -		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;

>> -		entry->eax = 0;

>> -		entry->ecx = F(RDPID);

>> -		++array->nent;

>> -	default:

>> +		entry.flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;

>> +		entry.ecx = F(RDPID);

>>   		break;

>> +	default:

>> +		goto out;

>>   	}

>>   

>> +	/* This check is performed only when func is valid */

> 

> Sorry to keep nitpicking and bikeshedding.  


No problem at all. Any comment is very welcome :)

Funcs aren't really "invalid", KVM
> just doesn't have any features it emulates in other leafs.  Maybe be more literal

> in describing what triggers the check?

> 

> 	/* Check the array capacity iff the entry is being copied over. */


What I mean here is that a func is "valid" if it matches one of the 
cases of the switch statement. If it is not valid, it ends up in the 
default case. But I agree, will change the comment your suggestion and 
resend.

Thank you,
Emanuele

> 

> Not a sticking point, so either way:

> 

> Reviewed-by: Sean Christopherson <seanjc@google.com>

> 

>> +	if (array->nent >= array->maxnent)

>> +		return -E2BIG;

>> +

>> +	entry.function = func;

>> +	memcpy(&array->entries[array->nent++], &entry, sizeof(entry));

>> +

>> +out:

>>   	return 0;

>>   }

>>   

>> -- 

>> 2.30.2

>>

>
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 6bd2f8b830e4..d30194081892 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -567,34 +567,33 @@  static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array,
 
 static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
 {
-	struct kvm_cpuid_entry2 *entry;
-
-	if (array->nent >= array->maxnent)
-		return -E2BIG;
+	struct kvm_cpuid_entry2 entry;
 
-	entry = &array->entries[array->nent];
-	entry->function = func;
-	entry->index = 0;
-	entry->flags = 0;
+	memset(&entry, 0, sizeof(entry));
 
 	switch (func) {
 	case 0:
-		entry->eax = 7;
-		++array->nent;
+		entry.eax = 7;
 		break;
 	case 1:
-		entry->ecx = F(MOVBE);
-		++array->nent;
+		entry.ecx = F(MOVBE);
 		break;
 	case 7:
-		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
-		entry->eax = 0;
-		entry->ecx = F(RDPID);
-		++array->nent;
-	default:
+		entry.flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+		entry.ecx = F(RDPID);
 		break;
+	default:
+		goto out;
 	}
 
+	/* This check is performed only when func is valid */
+	if (array->nent >= array->maxnent)
+		return -E2BIG;
+
+	entry.function = func;
+	memcpy(&array->entries[array->nent++], &entry, sizeof(entry));
+
+out:
 	return 0;
 }