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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <618d113d-e71a-076c-1ef8-4143110a5f39@redhat.com>
Date:   Thu, 5 Jan 2017 11:24:08 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Wanpeng Li <kernellwp@...il.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     Radim Krčmář <rkrcmar@...hat.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Wanpeng Li <wanpeng.li@...mail.com>,
        Alex Williamson <alex.williamson@...hat.com>
Subject: Re: [PATCH] KVM: eventfd: fix NULL deref irqbypass consumer



On 05/01/2017 10:05, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@...mail.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
>  
>