Re: [PATCH net] dibs: fix double free of dmb_clientid_arr

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The hardware interrupt handler `ism_handle_irq()` can execute out-of-order with respect to initialization and teardown of critical resources, leading to crashes (UAF, NULL deref, and uninitialized spinlock accesses).
- [High] `dibs_unregister_client()` leaves the subsystem in an inconsistent partial state if it aborts midway through its device iteration.
- [High] `dibs_lo_attach_dmb` drops the hash table read lock before incrementing the object's reference count, leading to a Use-After-Free.
--

--- Patch [1]: [PATCH net] dibs: fix double free of dmb_clientid_arr ---
commit 45565b8a1cec1239be540b3b54fad60822ae8b57
Author: Hidayath Khan <[email protected]>

dibs: fix double free of dmb_clientid_arr

dibs_dev_add() frees dibs->dmb_clientid_arr on both of its failure
paths, but does not clear the pointer. dibs_lo_dev_probe() then frees
the same pointer again in its err_reg path, so a failing device_add()
or sysfs_create_group() results in a double free.

Drop the kfree() from dibs_lo_dev_probe().

Cc: [email protected]

> diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
> index 0f2e093111526..aeb8787ea9816 100644
> --- a/drivers/dibs/dibs_loopback.c
> +++ b/drivers/dibs/dibs_loopback.c
> @@ -334,7 +334,6 @@ static int dibs_lo_dev_probe(void)
>  	return 0;
>  
>  err_reg:
> -	kfree(dibs->dmb_clientid_arr);

[Severity: High]
This isn't a bug introduced by this patch, but does dibs_lo_attach_dmb() have
a use-after-free if the reference count is incremented after dropping the hash
table lock?

drivers/dibs/dibs_loopback.c:dibs_lo_attach_dmb() {
...
	read_unlock_bh(&ldev->dmb_ht_lock);

	if (!refcount_inc_not_zero(&dmb_node->refcnt))
...
}

If a concurrent thread executing dibs_lo_unregister_dmb() acquires the lock
and drops the refcount to zero, could it free the node before the first thread
resumes, causing refcount_inc_not_zero() to operate on freed memory?

[Severity: High]
This is a pre-existing issue, but is there an initialization race with the
hardware interrupt handler in ism_drv.c?

drivers/s390/net/ism_drv.c:ism_probe() {
...
	ret = dibs_dev_add(dibs);
...
}

If the hardware interrupt is registered before dibs_dev_add() initializes
resources, and a spurious interrupt fires, could ism_handle_irq() access an
uninitialized spinlock or dereference a NULL pointer?

drivers/s390/net/ism_drv.c:ism_handle_irq() {
...
		client_id = dibs->dmb_clientid_arr[bit];
...
}

Similarly, during teardown:

drivers/s390/net/ism_drv.c:ism_remove() {
...
	dibs_dev_del(dibs);
	ism_dev_exit(ism);
...
}

Could freeing resources like dibs->dmb_clientid_arr via dibs_dev_del() before
the active IRQ is unregistered create a use-after-free window?

[Severity: High]
This is also a pre-existing issue, but does dibs_unregister_client() leave the
subsystem in an inconsistent state if it aborts early?

drivers/dibs/dibs_main.c:dibs_unregister_client() {
...
				rc = -EBUSY;
				goto err_reg_dmb;
...
}

If the loop successfully processes some devices but then encounters a device
with active DMBs, it returns -EBUSY without rolling back the teardown on the
previously processed devices. If the caller ignores this return value and
proceeds to unload the module, could the detached devices later trigger an
event and dereference freed ops function pointers?

>  	/* pairs with dibs_dev_alloc() */
>  	put_device(&dibs->dev);
>  	kfree(ldev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1