diff mbox series

KVM: eventfd: fix NULL deref irqbypass producer

Message ID 20230801085408.69597-1-likexu@tencent.com (mailing list archive)
State New, archived
Headers show
Series KVM: eventfd: fix NULL deref irqbypass producer | expand

Commit Message

Like Xu Aug. 1, 2023, 8:54 a.m. UTC 
From: Like Xu <likexu@tencent.com>

Adding guard logic to make irq_bypass_register/unregister_producer()
looks for the producer entry based on producer pointer itself instead
of pure token matching.

As was attempted commit 4f3dbdf47e15 ("KVM: eventfd: fix NULL deref
irqbypass consumer"), two different producers may occasionally have two
identical eventfd's. In this case, the later producer may unregister
the previous one after the registration fails (since they share the same
token), then NULL deref incurres in the path of deleting producer from
the producers list.

Registration should also fail if a registered producer changes its
token and registers again via the same producer pointer.

Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Like Xu <likexu@tencent.com>
---
 virt/lib/irqbypass.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: 5a7591176c47cce363c1eed704241e5d1c42c5a6

Comments

Alex Williamson Aug. 1, 2023, 8:26 p.m. UTC | #1 
On Tue,  1 Aug 2023 16:54:08 +0800
Like Xu <like.xu.linux@gmail.com> wrote:

> From: Like Xu <likexu@tencent.com>
> 
> Adding guard logic to make irq_bypass_register/unregister_producer()
> looks for the producer entry based on producer pointer itself instead
> of pure token matching.
> 
> As was attempted commit 4f3dbdf47e15 ("KVM: eventfd: fix NULL deref
> irqbypass consumer"), two different producers may occasionally have two
> identical eventfd's. In this case, the later producer may unregister
> the previous one after the registration fails (since they share the same
> token), then NULL deref incurres in the path of deleting producer from
> the producers list.
> 
> Registration should also fail if a registered producer changes its
> token and registers again via the same producer pointer.
> 
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  virt/lib/irqbypass.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Alex Williamson <alex.williamson@redhat.com>


> 
> diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
> index 28fda42e471b..e0aabbbf27ec 100644
> --- a/virt/lib/irqbypass.c
> +++ b/virt/lib/irqbypass.c
> @@ -98,7 +98,7 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer)
>  	mutex_lock(&lock);
>  
>  	list_for_each_entry(tmp, &producers, node) {
> -		if (tmp->token == producer->token) {
> +		if (tmp->token == producer->token || tmp == producer) {
>  			ret = -EBUSY;
>  			goto out_err;
>  		}
> @@ -148,7 +148,7 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer)
>  	mutex_lock(&lock);
>  
>  	list_for_each_entry(tmp, &producers, node) {
> -		if (tmp->token != producer->token)
> +		if (tmp != producer)
>  			continue;
>  
>  		list_for_each_entry(consumer, &consumers, node) {
> 
> base-commit: 5a7591176c47cce363c1eed704241e5d1c42c5a6
diff mbox series

Patch

diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
index 28fda42e471b..e0aabbbf27ec 100644
--- a/virt/lib/irqbypass.c
+++ b/virt/lib/irqbypass.c
@@ -98,7 +98,7 @@  int irq_bypass_register_producer(struct irq_bypass_producer *producer)
 	mutex_lock(&lock);
 
 	list_for_each_entry(tmp, &producers, node) {
-		if (tmp->token == producer->token) {
+		if (tmp->token == producer->token || tmp == producer) {
 			ret = -EBUSY;
 			goto out_err;
 		}
@@ -148,7 +148,7 @@  void irq_bypass_unregister_producer(struct irq_bypass_producer *producer)
 	mutex_lock(&lock);
 
 	list_for_each_entry(tmp, &producers, node) {
-		if (tmp->token != producer->token)
+		if (tmp != producer)
 			continue;
 
 		list_for_each_entry(consumer, &consumers, node) {