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

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

Pre-existing issues:
- [Critical] Hardware interrupt is registered before required data structures and locks are initialized.
- [Critical] Asymmetric teardown order creates a Use-After-Free window in `ism_remove()` and probe error paths.
- [High] Missing `class_unregister(&dibs_class)` on the error path in `dibs_init()`.
- [High] Incomplete rollback in `dibs_unregister_client()` leaves client in a partially unregistered state.
--

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

dibs: fix double free of dmb_clientid_arr

This commit fixes a double free of dibs->dmb_clientid_arr by dropping
the kfree() in the err_reg path of dibs_lo_dev_probe(). The pointer is
already freed by dibs_dev_add() on failure paths.

> diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
> index fd5caf1e19a8f..649e4e375be33 100644
> --- a/drivers/dibs/dibs_loopback.c
> +++ b/drivers/dibs/dibs_loopback.c
> @@ -335,7 +335,6 @@ static int dibs_lo_dev_probe(void)
>  	return 0;
>  
>  err_reg:
> -	kfree(dibs->dmb_clientid_arr);
>  	/* pairs with dibs_dev_alloc() */
>  	put_device(&dibs->dev);
>  	kfree(ldev);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a missing
class_unregister() on the error path in dibs_init(). 

If dibs_loopback_init() fails, it returns an error without cleaning up the
registered class:

drivers/dibs/dibs_main.c:dibs_init() {
    ...
	rc = dibs_loopback_init();
	if (rc)
		pr_err("%s fails with %d\n", __func__, rc);

	return rc;
}

Can this lead to a use-after-free? Any subsequent interaction with
/sys/class/dibs or reloading the module might dereference the freed class
struct.

[Severity: High]
This is a pre-existing issue, but incomplete rollback in
dibs_unregister_client() seems to leave the client in a partially
unregistered state:

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

If it encounters a device with active DMBs, it returns -EBUSY immediately
without restoring the state of previously unregistered devices. Does this
permanently corrupt the client state where it misses events for those
earlier devices while believing it is still fully registered?

[Severity: Critical]
This isn't a bug introduced by this patch, but the hardware interrupt
appears to be registered before the required data structures and locks are
initialized:

drivers/s390/net/ism_drv.c:ism_probe() {
    ...
	ret = ism_dev_init(ism);
	if (ret)
		goto err_dibs;
    ...
}

ism_dev_init() registers the hardware IRQ before dibs_dev_add() initializes
dibs->lock and dibs->dmb_clientid_arr. If a spurious or pending interrupt
fires during this small window:

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

Will this cause a panic due to attempting to acquire an uninitialized
spinlock and dereferencing the NULL dmb_clientid_arr pointer?

[Severity: Critical]
This is a pre-existing issue, but there seems to be an asymmetric teardown
order creating a use-after-free window in ism_remove():

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

dibs_dev_del() frees dibs->dmb_clientid_arr:

drivers/dibs/dibs_main.c:dibs_dev_del() {
    ...
	kfree(dibs->dmb_clientid_arr);
    ...
}

However, the interrupt is only disabled later in ism_dev_exit(). 

If an interrupt fires in this window, ism_handle_irq() will read from the
freed array. Could garbage data interpreted as client_id cause an
out-of-bounds access when indexing dibs->subs[client_id]?

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