Re: [PATCH v2] KVM: eventfd: fix NULL deref irqbypass consumer

[Paolo Bonzini]

On 06/01/2017 02:39, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Suggested-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>