Re: [PATCH v2 1/2] KVM: eventfd: Fix NULL deref irqbypass producer

[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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Like Xu <likexu@xxxxxxxxxxx>
> > Reviewed-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > ---
> >   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))