Re: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del()

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

Pre-existing issues:
- [Critical] Double free of `dmb_clientid_arr` in the error path of `dibs_lo_dev_probe()`.
- [Critical] The interrupt handler `ism_handle_irq()` can dereference NULL pointers and acquire an uninitialized lock due to early interrupt enablement.
- [Critical] Use-after-free of `ism->sba` and `ism->ieq` during teardown due to improper ordering with `free_irq()`.
- [High] Partial rollback failure leaves client in a broken state in `dibs_unregister_client()`.
--

--- Patch [1]: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() ---
commit 40d2232069f48423978af98bbecb5075fb611ca0
Author: Alexandra Winter <[email protected]>

net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del()
    
A dibs device interrupt handler can be active after dibs_dev_del() and
may still access dmb_clientid_arr. Free dmb_clientid_arr in
dibs_dev_release() after last reference is gone.

Fixes: cc21191b584c ("dibs: Move data path to dibs layer")
Cc: [email protected]
Co-developed-by: Hidayath Khan <[email protected]>
Signed-off-by: Hidayath Khan <[email protected]>
Signed-off-by: Alexandra Winter <[email protected]>

> --- a/drivers/dibs/dibs_main.c
> +++ b/drivers/dibs/dibs_main.c
> @@ -128,6 +128,7 @@ static void dibs_dev_release(struct device *dev)
>  
>  	dibs = container_of(dev, struct dibs_dev, dev);
>  
> +	kfree(dibs->dmb_clientid_arr);

[Severity: Critical]
This is a pre-existing issue, but does this lead to a double free in the
error path of dibs_lo_dev_probe()?

If device registration fails during loopback probe, the error path explicitly
frees the array and then puts the device:

drivers/dibs/dibs_loopback.c:dibs_lo_dev_probe() {
    ...
err_reg:
	kfree(dibs->dmb_clientid_arr);
	/* pairs with dibs_dev_alloc() */
	put_device(&dibs->dev);
    ...
}

Since put_device() now unconditionally calls dibs_dev_release() and frees
the array again, will this corrupt memory?

[ ... ]

> @@ -194,12 +195,13 @@ int dibs_dev_add(struct dibs_dev *dibs)

[Severity: Critical]
This isn't a bug introduced by this patch, but can the interrupt handler
access uninitialized structures if an early interrupt occurs?

In ism_dev_init(), interrupts are requested and enabled before the device
is fully registered:

drivers/s390/net/ism_drv.c:ism_dev_init() {
    ...
	ret = request_irq(pci_irq_vector(pdev, 0), ism_handle_irq, 0,
			  pci_name(pdev), ism);
	if (ret)
		goto free_vectors;

	ret = register_sba(ism);
    ...
}

If a hardware interrupt arrives immediately after request_irq(),
ism_handle_irq() will execute and unconditionally access the uninitialized
ism->sba and acquire dibs->lock, before register_sba() or dibs_dev_add()
have initialized them. Can this lead to a NULL pointer dereference or
uninitialized lock access?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does dibs_unregister_client() fail to
roll back its state if a client is busy?

If a client attempts to unregister while still having active DMBs on
multiple devices, the loop aborts and returns -EBUSY:

drivers/dibs/dibs_main.c:dibs_unregister_client() {
    ...
				rc = -EBUSY;
				goto err_reg_dmb;
			}
		}
		/* Stop forwarding IRQs and events */
		dibs->subs[client->id] = NULL;
		spin_unlock_irqrestore(&dibs->lock, flags);
		clients[client->id]->ops->del_dev(dibs);
    ...
}

Since it does not undo the modifications made to the preceding devices in
the list (such as clearing dibs->subs[client->id]), will this leave the
client in a broken state?


[Severity: Critical]
This isn't a bug introduced by this patch, but can a device interrupt cause
a use-after-free during driver removal?

In ism_dev_exit(), the data structures are destroyed before the interrupt
handler is quiesced:

drivers/s390/net/ism_drv.c:ism_dev_exit() {
	struct pci_dev *pdev = ism->pdev;

	unregister_ieq(ism);
	unregister_sba(ism);
	free_irq(pci_irq_vector(pdev, 0), ism);
    ...
}

If an interrupt arrives after unregister_sba() but before free_irq(),
ism_handle_irq() will access the freed ism->sba. Should free_irq() be
called before tearing down the data structures?

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