lkml.org 
[lkml]   [2023]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/2] KVM: eventfd: Fix NULL deref irqbypass producer
From
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))
>

\
 
 \ /
  Last update: 2023-09-25 10:00    [W:0.064 / U:0.448 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site