| Message ID | 20230802051700.52321-2-likexu@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: irqbypass: XArray conversion and a deref fix | expand |
On Wed, Aug 02, 2023, Like Xu 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> > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > --- > 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 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) What are the rules for using these APIs? E.g. is doing unregister without first doing a register actually allowed? Ditto for having multiple in-flight calls to (un)register the exact same producer or consumer. E.g. can we do something like the below, and then remove the list iteration to find the passed in pointer (which is super odd IMO). Obviously not a blocker for this patch, but it seems like we could achieve a simpler and more performant implementation if we first sanitize the rules and the usage. diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c index 28fda42e471b..be0ba4224a23 100644 --- a/virt/lib/irqbypass.c +++ b/virt/lib/irqbypass.c @@ -90,6 +90,9 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer) if (!producer->token) return -EINVAL; + if (WARN_ON_ONCE(producer->node.prev && !list_empty(&producer->node))) + return -EINVAL; + might_sleep(); if (!try_module_get(THIS_MODULE)) @@ -140,6 +143,9 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer) if (!producer->token) return; + if (WARN_ON_ONCE(!producer->node.prev || list_empty(&producer->node))) + return; + might_sleep(); if (!try_module_get(THIS_MODULE))
Hi Alex, what do you think of Sean's proposal diff below ? By the way, could anyone to accept patch 1/2, thanks. On 17/8/2023 2:37 am, Sean Christopherson wrote: > On Wed, Aug 02, 2023, Like Xu 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> >> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> >> --- >> 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 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) > > What are the rules for using these APIs? E.g. is doing unregister without > first doing a register actually allowed? Ditto for having multiple in-flight > calls to (un)register the exact same producer or consumer. > > E.g. can we do something like the below, and then remove the list iteration to > find the passed in pointer (which is super odd IMO). Obviously not a blocker > for this patch, but it seems like we could achieve a simpler and more performant > implementation if we first sanitize the rules and the usage. > > diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c > index 28fda42e471b..be0ba4224a23 100644 > --- a/virt/lib/irqbypass.c > +++ b/virt/lib/irqbypass.c > @@ -90,6 +90,9 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer) > if (!producer->token) > return -EINVAL; > > + if (WARN_ON_ONCE(producer->node.prev && !list_empty(&producer->node))) > + return -EINVAL; > + > might_sleep(); > > if (!try_module_get(THIS_MODULE)) > @@ -140,6 +143,9 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer) > if (!producer->token) > return; > > + if (WARN_ON_ONCE(!producer->node.prev || list_empty(&producer->node))) > + return; > + > might_sleep(); > > if (!try_module_get(THIS_MODULE)) >
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) {