Re: [PATCH net] dibs: fix use-after-free of dmb_node in loopback attach/detach/unregister

Thanks for the Sashiko AI review.

On 28/07/26 3:05 pm, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] dibs_lo_detach_dmb() fails to clear dibs->dmb_clientid_arr when dropping the final reference, leading to permanent client resource leaks.
> - [High] Double free of dibs->dmb_clientid_arr in dibs_lo_dev_probe() error path.
Confirmed as a real double-free. The patch for this is already in the 
internal review.
> - [High] Race condition in dibs_lo_move_data() routes spurious interrupts to wrong clients.
> --
>
> --- Patch [1]: [PATCH net] dibs: fix use-after-free of dmb_node in loopback attach/detach/unregister ---
> commit 90d8ce044f1442abf2a546d5baabbf36276d4b45
> Author: Hidayath Khan <[email protected]>
>
> dibs: fix use-after-free of dmb_node in loopback attach/detach/unregister
>
> This commit fixes a use-after-free race condition in the DMB loopback
> operations. It closes the race by making the hash table membership and refcount
> transitions atomic with respect to each other under the write lock. Freeing the
> node is now handled safely after the lock is dropped.
>
>> diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
>> index 0f2e093111526..fd5caf1e19a8f 100644
>> --- a/drivers/dibs/dibs_loopback.c
>> +++ b/drivers/dibs/dibs_loopback.c
> [ ... ]
>> @@ -209,11 +205,12 @@ static int dibs_lo_detach_dmb(struct dibs_dev *dibs, u64 token)
>>   {
> [ ... ]
>> +	last = refcount_dec_and_test(&dmb_node->refcnt);
>> +	if (last)
>> +		hash_del(&dmb_node->list);
>> +	write_unlock_bh(&ldev->dmb_ht_lock);
>> +
>> +	if (last)
>> +		dibs_lo_free_dmb(ldev, dmb_node);
> [Severity: High]
> This isn't a bug introduced by this patch, but does this code fail to clear
> the client ID array when dropping the final reference?
>
> When an owner unregisters a DMB that still has active attachers (refcount > 1),
> dibs_lo_unregister_dmb() does not clear the array because last == false.
>
> When the final attacher detaches, dibs_lo_detach_dmb() drops the refcount to 0,
> frees the node and the sba_idx, but structurally fails to clear
> dibs->dmb_clientid_arr[sba_idx].
>
> Could this leave a stale client ID in dmb_clientid_arr indefinitely, causing
> future unregistration to fail with -EBUSY and permanently leaking the client
> resource?
>
>>   
>> -	if (refcount_dec_and_test(&dmb_node->refcnt))
>> -		__dibs_lo_unregister_dmb(ldev, dmb_node);
>>   	return 0;
>>   }
> [Severity: High]
> This is a pre-existing issue, but is there a double free of
> dibs->dmb_clientid_arr in the dibs_lo_dev_probe() error path?
>
> If dibs_dev_add() fails internally, it frees the array without nullifying it:
>
> drivers/dibs/dibs_main.c:dibs_dev_add() {
>      ...
> free_client_arr:
> 	kfree(dibs->dmb_clientid_arr);
> 	return ret;
> }
>
> The caller receives the error and unconditionally frees the already-freed array
> again:
>
> drivers/dibs/dibs_loopback.c:dibs_lo_dev_probe() {
>      ...
> err_reg:
> 	kfree(dibs->dmb_clientid_arr);
>      ...
> }
>
> Could this double-free cause slab allocator corruption?
>
> [Severity: High]
> This is a pre-existing issue, but is there a race condition in
> dibs_lo_move_data() that routes spurious interrupts to wrong clients?
>
> The target sba_idx is cached and the protection lock is dropped:
>
> drivers/dibs/dibs_loopback.c:dibs_lo_move_data() {
>      ...
> 	sba_idx = rmb_node->sba_idx;
> 	read_unlock_bh(&ldev->dmb_ht_lock);
>      ...
> }
>
> Before it acquires the lock to trigger the IRQ, another CPU could drop the
> final reference to the DMB, free the node, and clear its sba_idx.
>
> If a completely different client then immediately registers a new DMB and
> reallocates the same sba_idx, will the IRQ be routed to the wrong client when
> the code continues?
>
> drivers/dibs/dibs_loopback.c:dibs_lo_move_data() {
>      ...
> 	spin_lock(&dibs->lock);
> 	client_id = dibs->dmb_clientid_arr[sba_idx];
> 	s_mask = ror16(0x1000, idx);
> 	if (likely(client_id != NO_DIBS_CLIENT && dibs->subs[client_id]))
> 		dibs->subs[client_id]->ops->handle_irq(dibs, sba_idx, s_mask);
>      ...
> }
>