| Message ID | 1483666782-2693-1-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/01/2017 02:39, Wanpeng Li wrote: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > Reported syzkaller: > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008 > IP: irq_bypass_unregister_consumer+0x9d/0xb70 [irqbypass] > PGD 0 > > Oops: 0002 [#1] SMP > CPU: 1 PID: 125 Comm: kworker/1:1 Not tainted 4.9.0+ #1 > Workqueue: kvm-irqfd-cleanup irqfd_shutdown [kvm] > task: ffff9bbe0dfbb900 task.stack: ffffb61802014000 > RIP: 0010:irq_bypass_unregister_consumer+0x9d/0xb70 [irqbypass] > Call Trace: > irqfd_shutdown+0x66/0xa0 [kvm] > process_one_work+0x16b/0x480 > worker_thread+0x4b/0x500 > kthread+0x101/0x140 > ? process_one_work+0x480/0x480 > ? kthread_create_on_node+0x60/0x60 > ret_from_fork+0x25/0x30 > RIP: irq_bypass_unregister_consumer+0x9d/0xb70 [irqbypass] RSP: ffffb61802017e20 > CR2: 0000000000000008 > > The syzkaller folks reported a NULL pointer dereference that due to > unregister an consumer which fails registration before. The syzkaller > creates two VMs w/ an equal eventfd occasionally. So the second VM > fails to register an irqbypass consumer. It will make irqfd as inactive > and queue an workqueue work to shutdown irqfd and unregister the irqbypass > consumer when eventfd is closed. However, the second consumer has been > initialized though it fails registration. So the token(same as the first > VM's) is taken to unregister the consumer through the workqueue, the > consumer of the first VM is found and unregistered, then NULL deref incurred > in the path of deleting consumer from the consumers list. > > #include <fcntl.h> > #include <pthread.h> > #include <setjmp.h> > #include <signal.h> > #include <stddef.h> > #include <stdint.h> > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > #include <sys/ioctl.h> > #include <sys/stat.h> > #include <sys/syscall.h> > #include <sys/types.h> > #include <unistd.h> > > __thread int skip_segv; > __thread jmp_buf segv_env; > > static void segv_handler(int sig, siginfo_t* info, void* uctx) > { > if (__atomic_load_n(&skip_segv, __ATOMIC_RELAXED)) > _longjmp(segv_env, 1); > exit(sig); > } > > static void install_segv_handler() > { > struct sigaction sa; > memset(&sa, 0, sizeof(sa)); > sa.sa_sigaction = segv_handler; > sa.sa_flags = SA_NODEFER | SA_SIGINFO; > sigaction(SIGSEGV, &sa, NULL); > sigaction(SIGBUS, &sa, NULL); > } > > #define NONFAILING(...) \ > { \ > __atomic_fetch_add(&skip_segv, 1, __ATOMIC_SEQ_CST); \ > if (_setjmp(segv_env) == 0) { \ > __VA_ARGS__; \ > } \ > __atomic_fetch_sub(&skip_segv, 1, __ATOMIC_SEQ_CST); \ > } > > static uintptr_t execute_syscall(int nr, uintptr_t a0, uintptr_t a1, > uintptr_t a2, uintptr_t a3, > uintptr_t a4, uintptr_t a5, > uintptr_t a6, uintptr_t a7, > uintptr_t a8) > { > return syscall(nr, a0, a1, a2, a3, a4, a5); > } > > long r[28]; > void* thr(void* arg) > { > switch ((long)arg) { > case 0: > r[0] = > execute_syscall(__NR_mmap, 0x20000000ul, 0xd000ul, 0x3ul, > 0x32ul, 0xfffffffffffffffful, 0x0ul, 0, 0, 0); > break; > case 1: > r[2] = syscall(__NR_open, "/dev/kvm", 0x40042ul, 0, 0, 0, 0, 0, 0); > break; > case 2: > r[3] = execute_syscall(__NR_ioctl, r[2], 0xae01ul, 0x0ul, 0, 0, 0, > 0, 0, 0); > break; > case 3: > r[4] = execute_syscall(__NR_ioctl, r[3], 0xae41ul, 0x3fful, 0, 0, 0, > 0, 0, 0); > break; > case 4: > r[5] = execute_syscall(__NR_ioctl, r[4], 0xae9aul, 0, 0, 0, 0, 0, 0, > 0); > break; > case 5: > r[6] = execute_syscall(__NR_eventfd2, 0x8ul, 0x801ul, 0, 0, 0, 0, 0, > 0, 0); > break; > case 6: > NONFAILING(*(uint32_t*)0x2000c000 = r[6]); > NONFAILING(*(uint32_t*)0x2000c004 = (uint32_t)0x98cd); > NONFAILING(*(uint32_t*)0x2000c008 = (uint32_t)0x0); > NONFAILING(*(uint32_t*)0x2000c00c = (uint32_t)0xffffffffffffffff); > NONFAILING(*(uint8_t*)0x2000c010 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c011 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c012 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c013 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c014 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c015 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c016 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c017 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c018 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c019 = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c01a = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c01b = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c01c = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c01d = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c01e = (uint8_t)0x0); > NONFAILING(*(uint8_t*)0x2000c01f = (uint8_t)0x0); > r[27] = execute_syscall(__NR_ioctl, r[3], 0x4020ae76ul, > 0x2000c000ul, 0, 0, 0, 0, 0, 0); > break; > } > return 0; > } > > int main() > { > long i; > pthread_t th[14]; > > install_segv_handler(); > memset(r, -1, sizeof(r)); > srand(getpid()); > for (i = 0; i < 7; i++) { > pthread_create(&th[i], 0, thr, (void*)i); > usleep(10000); > } > for (i = 0; i < 7; i++) { > pthread_create(&th[7 + i], 0, thr, (void*)i); > if (rand() % 2) > usleep(rand() % 10000); > } > usleep(100000); > return 0; > } > > This patch fixes it by making irq_bypass_register/unregister_consumer() > looks for the consumer entry based on consumer pointer itself instead of > token matching. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > --- > v1 -> v2: > * make irq_bypass_register/unregister_consumer() looks for the consumer > entry based on consumer pointer itself instead of token matching > > virt/lib/irqbypass.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c > index 52abac4..6d2fcd6 100644 > --- a/virt/lib/irqbypass.c > +++ b/virt/lib/irqbypass.c > @@ -195,7 +195,7 @@ int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer) > mutex_lock(&lock); > > list_for_each_entry(tmp, &consumers, node) { > - if (tmp->token == consumer->token) { > + if (tmp->token == consumer->token || tmp == consumer) { > mutex_unlock(&lock); > module_put(THIS_MODULE); > return -EBUSY; > @@ -245,7 +245,7 @@ void irq_bypass_unregister_consumer(struct irq_bypass_consumer *consumer) > mutex_lock(&lock); > > list_for_each_entry(tmp, &consumers, node) { > - if (tmp->token != consumer->token) > + if (tmp != consumer) > continue; > > list_for_each_entry(producer, &producers, node) { > Acked-by: Paolo Bonzini <pbonzini@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c index 52abac4..6d2fcd6 100644 --- a/virt/lib/irqbypass.c +++ b/virt/lib/irqbypass.c @@ -195,7 +195,7 @@ int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer) mutex_lock(&lock); list_for_each_entry(tmp, &consumers, node) { - if (tmp->token == consumer->token) { + if (tmp->token == consumer->token || tmp == consumer) { mutex_unlock(&lock); module_put(THIS_MODULE); return -EBUSY; @@ -245,7 +245,7 @@ void irq_bypass_unregister_consumer(struct irq_bypass_consumer *consumer) mutex_lock(&lock); list_for_each_entry(tmp, &consumers, node) { - if (tmp->token != consumer->token) + if (tmp != consumer) continue; list_for_each_entry(producer, &producers, node) {