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

On 28.07.26 11:35, [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.
> - [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.
> 


This is a true statement for future dibs clients (=dmb owners), but the current only
dibs client smc does not do that. All dibs-loopback dmbs are detached before they
are unregistered. -> not an issue today.

I will follow up to make this more future-proof.


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

Ack. Hidayath will send a fix soon.

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

Not an issue today, because smc is the only dibs-client.
But definitively needs improvement. I will follow up.


> 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);
>     ...
> }
>