Messages in this thread | | | Date | Mon, 25 Sep 2023 15:59:40 +0800 | Subject | Re: [PATCH v2 1/2] KVM: eventfd: Fix NULL deref irqbypass producer | From | Like Xu <> |
| |
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)) >
| |