Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <74dc2c4a-077d-a538-4c4d-9f2141702ada@gmail.com>
Date:   Mon, 25 Sep 2023 15:59:40 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.15.1
Subject: Re: [PATCH v2 1/2] KVM: eventfd: Fix NULL deref irqbypass producer
Content-Language: en-US
To:     Alex Williamson <alex.williamson@redhat.com>,
        Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
References: <20230802051700.52321-1-likexu@tencent.com>
 <20230802051700.52321-2-likexu@tencent.com> <ZN0XXKezcXjv1GWH@google.com>
From:   Like Xu <like.xu.linux@gmail.com>
In-Reply-To: <ZN0XXKezcXjv1GWH@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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))
>