[PATCH v2 1/2] KVM: cpuid: Fix read/write out-of-bounds vulnerability in cpuid emulation

[PATCH v2 1/2] KVM: cpuid: Fix read/write out-of-bounds vulnerability in cpuid emulation

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

If "i" is the last element in the vcpu->arch.cpuid_entries[] array, it 
potentially can be exploited the vulnerability. this will out-of-bounds 
read and write the unused memory in host OS.

As Paolo pointed:

>>  	/* when no next entry is found, the current entry[i] is reselected */
>> -	for (j = i + 1; ; j = (j + 1) % nent) {
>> -		struct kvm_cpuid_entry2 *ej = &vcpu->arch.cpuid_entries[j];
>> -		if (ej->function == e->function) {
>
>It reads ej->maxphyaddr, which is user controlled.
>
>> -			ej->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
>
>After cpuid_entries there is
>
>        int maxphyaddr;
>        struct x86_emulate_ctxt emulate_ctxt;  /* 16-byte aligned */
>
>So indeed we have:
>
>- cpuid_entries at offset 1B50 (6992)
>- maxphyaddr at offset 27D0 (6992 + 3200 = 10192)
>- padding at 27D4...27DF
>- emulate_ctxt at 27E0
>
>So this indeed writes in the padding.  Pfew, writing the ops field of
>emulate_ctxt would have been much worse.

This patch fixes it by modding the index to avoid the out-of-bounds. At 
the worst case, i == j and ej->function == e->function, the loop can bail
out.

Reported-by: Moguofang <moguofang@huawei.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Moguofang <moguofang@huawei.com>
Cc: stable@vger.kernel.org
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v1 -> v2:
 * update patch description

 arch/x86/kvm/cpuid.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index a181ae7..b927a42 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -779,19 +779,20 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
 
 static int move_to_next_stateful_cpuid_entry(struct kvm_vcpu *vcpu, int i)
 {
+	int j = i, nent = vcpu->arch.cpuid_nent;
 	struct kvm_cpuid_entry2 *e = &vcpu->arch.cpuid_entries[i];
-	int j, nent = vcpu->arch.cpuid_nent;
+	struct kvm_cpuid_entry2 *ej;
 
 	e->flags &= ~KVM_CPUID_FLAG_STATE_READ_NEXT;
 	/* when no next entry is found, the current entry[i] is reselected */
-	for (j = i + 1; ; j = (j + 1) % nent) {
-		struct kvm_cpuid_entry2 *ej = &vcpu->arch.cpuid_entries[j];
-		if (ej->function == e->function) {
-			ej->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
-			return j;
-		}
-	}
-	return 0; /* silence gcc, even though control never reaches here */
+	do {
+		j = (j + 1) % nent;
+		ej = &vcpu->arch.cpuid_entries[j];
+	} while(ej->function != e->function);
+
+	ej->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
+
+	return j;
 }
 
 /* find an entry with matching function, matching index (if needed), and that
-- 
2.7.4

[PATCH v2 2/2] KVM: async_pf: rcu irq exit if not triggered from idle task

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Commit 9b132fbe5419 (Add rcu user eqs exception hooks for async page fault) 
adds rcu_irq_enter/exit() to kvm_async_pf_task_wait() to exit cpu idle eqs 
when needed, to protect the code that needs use rcu. There is no need to call 
this pairs if async page fault is not triggered from idle task.

This patch invokes rcu irq exit if it is not triggered from idle task.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v1 -> v2:
 * update patch description

 arch/x86/kernel/kvm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 43e10d6..e70ed72 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -141,6 +141,8 @@ void kvm_async_pf_task_wait(u32 token)
 	n.token = token;
 	n.cpu = smp_processor_id();
 	n.halted = is_idle_task(current) || preempt_count() > 1;
+	if (!n.halted)
+		rcu_irq_exit();
 	init_swait_queue_head(&n.wq);
 	hlist_add_head(&n.link, &b->list);
 	raw_spin_unlock(&b->lock);
@@ -167,8 +169,9 @@ void kvm_async_pf_task_wait(u32 token)
 	}
 	if (!n.halted)
 		finish_swait(&n.wq, &wait);
+	else
+		rcu_irq_exit();
 
-	rcu_irq_exit();
 	return;
 }
 EXPORT_SYMBOL_GPL(kvm_async_pf_task_wait);
-- 
2.7.4

Re: [PATCH v2 1/2] KVM: cpuid: Fix read/write out-of-bounds vulnerability in cpuid emulation

[Paolo Bonzini]

On 08/06/2017 10:22, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> If "i" is the last element in the vcpu->arch.cpuid_entries[] array, it 
> potentially can be exploited the vulnerability. this will out-of-bounds 
> read and write the unused memory in host OS.
> 
> As Paolo pointed:
> 
>>>  	/* when no next entry is found, the current entry[i] is reselected */
>>> -	for (j = i + 1; ; j = (j + 1) % nent) {
>>> -		struct kvm_cpuid_entry2 *ej = &vcpu->arch.cpuid_entries[j];
>>> -		if (ej->function == e->function) {
>>
>> It reads ej->maxphyaddr, which is user controlled.
>>
>>> -			ej->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
>>
>> After cpuid_entries there is
>>
>>        int maxphyaddr;
>>        struct x86_emulate_ctxt emulate_ctxt;  /* 16-byte aligned */
>>
>> So indeed we have:
>>
>> - cpuid_entries at offset 1B50 (6992)
>> - maxphyaddr at offset 27D0 (6992 + 3200 = 10192)
>> - padding at 27D4...27DF
>> - emulate_ctxt at 27E0
>>
>> So this indeed writes in the padding.  Pfew, writing the ops field of
>> emulate_ctxt would have been much worse.
> 
> This patch fixes it by modding the index to avoid the out-of-bounds. At 
> the worst case, i == j and ej->function == e->function, the loop can bail
> out.
> 
> Reported-by: Moguofang <moguofang@huawei.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Moguofang <moguofang@huawei.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v1 -> v2:
>  * update patch description

Queued, thanks.

Paolo

> 
>  arch/x86/kvm/cpuid.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a181ae7..b927a42 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -779,19 +779,20 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>  
>  static int move_to_next_stateful_cpuid_entry(struct kvm_vcpu *vcpu, int i)
>  {
> +	int j = i, nent = vcpu->arch.cpuid_nent;
>  	struct kvm_cpuid_entry2 *e = &vcpu->arch.cpuid_entries[i];
> -	int j, nent = vcpu->arch.cpuid_nent;
> +	struct kvm_cpuid_entry2 *ej;
>  
>  	e->flags &= ~KVM_CPUID_FLAG_STATE_READ_NEXT;
>  	/* when no next entry is found, the current entry[i] is reselected */
> -	for (j = i + 1; ; j = (j + 1) % nent) {
> -		struct kvm_cpuid_entry2 *ej = &vcpu->arch.cpuid_entries[j];
> -		if (ej->function == e->function) {
> -			ej->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
> -			return j;
> -		}
> -	}
> -	return 0; /* silence gcc, even though control never reaches here */
> +	do {
> +		j = (j + 1) % nent;
> +		ej = &vcpu->arch.cpuid_entries[j];
> +	} while(ej->function != e->function);
> +
> +	ej->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
> +
> +	return j;
>  }
>  
>  /* find an entry with matching function, matching index (if needed), and that
>