Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1550390 > unrolled thread

[PATCH v2] KVM: x86: fix NULL deref in vcpu_scan_ioapic

Started byWanpeng Li <kernellwp@gmail.com>
First post2017-01-04 04:00 +0100
Last post2017-01-12 15:00 +0100
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] KVM: x86: fix NULL deref in vcpu_scan_ioapic Wanpeng Li <kernellwp@gmail.com> - 2017-01-04 04:00 +0100
    Re: [PATCH v2] KVM: x86: fix NULL deref in vcpu_scan_ioapic David Hildenbrand <david@redhat.com> - 2017-01-04 10:40 +0100
      Re: [PATCH v2] KVM: x86: fix NULL deref in vcpu_scan_ioapic Paolo Bonzini <pbonzini@redhat.com> - 2017-01-12 15:00 +0100

#1550390 — [PATCH v2] KVM: x86: fix NULL deref in vcpu_scan_ioapic

FromWanpeng Li <kernellwp@gmail.com>
Date2017-01-04 04:00 +0100
Subject[PATCH v2] KVM: x86: fix NULL deref in vcpu_scan_ioapic
Message-ID<sVJXX-6QN-5@gated-at.bofh.it>
From: Wanpeng Li <wanpeng.li@hotmail.com>

Reported by syzkaller:

    BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
    IP: _raw_spin_lock+0xc/0x30
    PGD 3e28eb067
    PUD 3f0ac6067
    PMD 0
    Oops: 0002 [#1] SMP
    CPU: 0 PID: 2431 Comm: test Tainted: G           OE   4.10.0-rc1+ #3
    Call Trace:
     ? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
     kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
     ? pick_next_task_fair+0xe1/0x4e0
     ? kvm_arch_vcpu_load+0xea/0x260 [kvm]
     kvm_vcpu_ioctl+0x33a/0x600 [kvm]
     ? hrtimer_try_to_cancel+0x29/0x130
     ? do_nanosleep+0x97/0xf0
     do_vfs_ioctl+0xa1/0x5d0
     ? __hrtimer_init+0x90/0x90
     ? do_nanosleep+0x5b/0xf0
     SyS_ioctl+0x79/0x90
     do_syscall_64+0x6e/0x180
     entry_SYSCALL64_slow_path+0x25/0x25
    RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0

The syzkaller folks reported a NULL pointer dereference that due to 
ENABLE_CAP doesn't fail without an irqchip, and synic is activated 
which results in a rescan ioapic request has been set although it 
should never been set for that vCPU.

#include <sys/ioctl.h> 
#include <sys/mman.h> 
#include <sys/types.h> 
#include <linux/kvm.h> 
#include <pthread.h> 
#include <stddef.h> 
#include <stdint.h> 
#include <stdlib.h> 
#include <string.h> 
#include <unistd.h> 

#ifndef KVM_CAP_HYPERV_SYNIC 
#define KVM_CAP_HYPERV_SYNIC 123 
#endif 

void* thr(void* arg) 
{ 
	struct kvm_enable_cap cap; 
	cap.flags = 0; 
	cap.cap = KVM_CAP_HYPERV_SYNIC; 
	ioctl((long)arg, KVM_ENABLE_CAP, &cap); 
	return 0; 
} 

int main() 
{ 
	void *host_mem = mmap(0, 0x1000, PROT_READ|PROT_WRITE, 
			MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); 
	int kvmfd = open("/dev/kvm", 0); 
	int vmfd = ioctl(kvmfd, KVM_CREATE_VM, 0); 
	struct kvm_userspace_memory_region memreg; 
	memreg.slot = 0; 
	memreg.flags = 0; 
	memreg.guest_phys_addr = 0; 
	memreg.memory_size = 0x1000; 
	memreg.userspace_addr = (unsigned long)host_mem; 
	memcpy(host_mem, 
			"\x1a\xb5\x00\x04\x65\x26\x64\x26\xf0\x82\xaa\x00" 
			"\x02\x00\x3e\x75\x64\xf3\xf3\xf0\x18\x35\x66\xb9" 
			"\x99\x00\x00\x40\x46\xb8\xc0\x40\x00\x00\x66\xba" 
			"\x00\x00\x00\x00\x0f\x30\x0f\xfc\xda\xdb\xca\x36" 
			"\x26\x3e\x67\x3e\x67\xcf\x66\xb9\x4a\x0a\x00\x00" 
			"\x0f\x32\x2e\x26\x65\x0f\x0f\x01\xa0", 
			69); 
	ioctl(vmfd, KVM_SET_USER_MEMORY_REGION, &memreg); 
	int cpufd = ioctl(vmfd, KVM_CREATE_VCPU, 0); 
	struct kvm_sregs sregs; 
	ioctl(cpufd, KVM_GET_SREGS, &sregs); 
	sregs.cr0 = 0; 
	sregs.cr4 = 0; 
	sregs.efer = 0; 
	sregs.cs.selector = 0; 
	sregs.cs.base = 0; 
	ioctl(cpufd, KVM_SET_SREGS, &sregs); 
	struct kvm_regs regs; 
	memset(&regs, 0, sizeof(regs)); 
	regs.rflags = 2; 
	ioctl(cpufd, KVM_SET_REGS, &regs); 
	ioctl(vmfd, KVM_CREATE_IRQCHIP, 0); 
	pthread_t th; 
	pthread_create(&th, 0, thr, (void*)(long)cpufd); 
	usleep(rand() % 10000); 
	ioctl(cpufd, KVM_RUN, 0); 
	pthread_join(th, 0); 
	return 0; 
} 

This patch fix it by failing ENABLE_CAP if without an irqchip.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v1 -> v2:
 * fail ENABLE_CAP if without an irqchip

 arch/x86/kvm/x86.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5f6e288..2ada44a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3345,6 +3345,8 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
 
 	switch (cap->cap) {
 	case KVM_CAP_HYPERV_SYNIC:
+		if (!irqchip_in_kernel(vcpu->kvm))
+			return -EINVAL;
 		return kvm_hv_activate_synic(vcpu);
 	default:
 		return -EINVAL;
-- 
2.7.4

[toc] | [next] | [standalone]

#1550574

FromDavid Hildenbrand <david@redhat.com>
Date2017-01-04 10:40 +0100
Message-ID<sVQd3-2M8-17@gated-at.bofh.it>
In reply to#1550390
Am 04.01.2017 um 03:56 schrieb Wanpeng Li:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
> Reported by syzkaller:
>
>     BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
>     IP: _raw_spin_lock+0xc/0x30
>     PGD 3e28eb067
>     PUD 3f0ac6067
>     PMD 0
>     Oops: 0002 [#1] SMP
>     CPU: 0 PID: 2431 Comm: test Tainted: G           OE   4.10.0-rc1+ #3
>     Call Trace:
>      ? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
>      kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
>      ? pick_next_task_fair+0xe1/0x4e0
>      ? kvm_arch_vcpu_load+0xea/0x260 [kvm]
>      kvm_vcpu_ioctl+0x33a/0x600 [kvm]
>      ? hrtimer_try_to_cancel+0x29/0x130
>      ? do_nanosleep+0x97/0xf0
>      do_vfs_ioctl+0xa1/0x5d0
>      ? __hrtimer_init+0x90/0x90
>      ? do_nanosleep+0x5b/0xf0
>      SyS_ioctl+0x79/0x90
>      do_syscall_64+0x6e/0x180
>      entry_SYSCALL64_slow_path+0x25/0x25
>     RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0
>
> The syzkaller folks reported a NULL pointer dereference that due to
> ENABLE_CAP doesn't fail without an irqchip, and synic is activated
> which results in a rescan ioapic request has been set although it
> should never been set for that vCPU.
>
> #include <sys/ioctl.h>
> #include <sys/mman.h>
> #include <sys/types.h>
> #include <linux/kvm.h>
> #include <pthread.h>
> #include <stddef.h>
> #include <stdint.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
>
> #ifndef KVM_CAP_HYPERV_SYNIC
> #define KVM_CAP_HYPERV_SYNIC 123
> #endif
>
> void* thr(void* arg)
> {
> 	struct kvm_enable_cap cap;
> 	cap.flags = 0;
> 	cap.cap = KVM_CAP_HYPERV_SYNIC;
> 	ioctl((long)arg, KVM_ENABLE_CAP, &cap);
> 	return 0;
> }
>
> int main()
> {
> 	void *host_mem = mmap(0, 0x1000, PROT_READ|PROT_WRITE,
> 			MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> 	int kvmfd = open("/dev/kvm", 0);
> 	int vmfd = ioctl(kvmfd, KVM_CREATE_VM, 0);
> 	struct kvm_userspace_memory_region memreg;
> 	memreg.slot = 0;
> 	memreg.flags = 0;
> 	memreg.guest_phys_addr = 0;
> 	memreg.memory_size = 0x1000;
> 	memreg.userspace_addr = (unsigned long)host_mem;
> 	memcpy(host_mem,
> 			"\x1a\xb5\x00\x04\x65\x26\x64\x26\xf0\x82\xaa\x00"
> 			"\x02\x00\x3e\x75\x64\xf3\xf3\xf0\x18\x35\x66\xb9"
> 			"\x99\x00\x00\x40\x46\xb8\xc0\x40\x00\x00\x66\xba"
> 			"\x00\x00\x00\x00\x0f\x30\x0f\xfc\xda\xdb\xca\x36"
> 			"\x26\x3e\x67\x3e\x67\xcf\x66\xb9\x4a\x0a\x00\x00"
> 			"\x0f\x32\x2e\x26\x65\x0f\x0f\x01\xa0",
> 			69);
> 	ioctl(vmfd, KVM_SET_USER_MEMORY_REGION, &memreg);
> 	int cpufd = ioctl(vmfd, KVM_CREATE_VCPU, 0);
> 	struct kvm_sregs sregs;
> 	ioctl(cpufd, KVM_GET_SREGS, &sregs);
> 	sregs.cr0 = 0;
> 	sregs.cr4 = 0;
> 	sregs.efer = 0;
> 	sregs.cs.selector = 0;
> 	sregs.cs.base = 0;
> 	ioctl(cpufd, KVM_SET_SREGS, &sregs);
> 	struct kvm_regs regs;
> 	memset(&regs, 0, sizeof(regs));
> 	regs.rflags = 2;
> 	ioctl(cpufd, KVM_SET_REGS, &regs);
> 	ioctl(vmfd, KVM_CREATE_IRQCHIP, 0);
> 	pthread_t th;
> 	pthread_create(&th, 0, thr, (void*)(long)cpufd);
> 	usleep(rand() % 10000);
> 	ioctl(cpufd, KVM_RUN, 0);
> 	pthread_join(th, 0);
> 	return 0;
> }
>
> This patch fix it by failing ENABLE_CAP if without an irqchip.

Not sure if that reproducer should go in there, at least for me
the description of the problem is sufficient.

And I think you should add

Fixes: 5c919412fe61 (kvm/x86: Hyper-V synthetic interrupt controller)
Cc: stable@vger.kernel.org # 4.5+

I'll put Andrey on cc.

Reviewed-by: David Hildenbrand <david@redhat.com>

>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v1 -> v2:
>  * fail ENABLE_CAP if without an irqchip
>
>  arch/x86/kvm/x86.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5f6e288..2ada44a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3345,6 +3345,8 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>
>  	switch (cap->cap) {
>  	case KVM_CAP_HYPERV_SYNIC:
> +		if (!irqchip_in_kernel(vcpu->kvm))
> +			return -EINVAL;
>  		return kvm_hv_activate_synic(vcpu);
>  	default:
>  		return -EINVAL;
>


-- 

David

[toc] | [prev] | [next] | [standalone]

#1557466

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-01-12 15:00 +0100
Message-ID<sYO53-4DI-7@gated-at.bofh.it>
In reply to#1550574

On 04/01/2017 10:30, David Hildenbrand wrote:
> Fixes: 5c919412fe61 (kvm/x86: Hyper-V synthetic interrupt controller)
> Cc: stable@vger.kernel.org # 4.5+

Added both and queued.

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web