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

[Paolo Bonzini <pbonzini@redhat.com>]

On 05/01/2017 10:05, 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 occassionally. 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 in 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.

Thanks Wanpend!

However, I'm wondering if we should improve the API instead.  Maybe the
token should be passed to irq_bypass_register_{producer,consumer} and
only assigned if the functions succeed.  Alex, what do you think?

Thanks,

Paolo

> 
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index a29786d..eeaf056 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -415,9 +415,15 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
>  		irqfd->consumer.stop = kvm_arch_irq_bypass_stop;
>  		irqfd->consumer.start = kvm_arch_irq_bypass_start;
>  		ret = irq_bypass_register_consumer(&irqfd->consumer);
> -		if (ret)
> +		if (ret) {
>  			pr_info("irq bypass consumer (token %p) registration fails: %d\n",
>  				irqfd->consumer.token, ret);
> +			irqfd->consumer.token = NULL;
> +			irqfd->consumer.add_producer = NULL;
> +			irqfd->consumer.del_producer = NULL;
> +			irqfd->consumer.stop = NULL;
> +			irqfd->consumer.start = NULL;
> +		}
>  	}
>  #endif
>  
>