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

[sashiko-bot@kernel.org]

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 <hidayath@linux.ibm.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727093530.968834-1-hidayath@linux.ibm.com?part=1