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

On 05.08.26 10:59, [email protected] wrote:
> 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?
> 
> [ ... ]
> 

[PATCH net] dibs: fix double free of dmb_clientid_arr
is in discussion on the netdev mailinglist already.


>> @@ -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?
> 
> [ ... ]

Cannot happen.
ism hardware will not generate interrupts before SBA AND IEQ are registered.
A cleanup patch is planned.


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

There is no such codepath with today's SMC client.
I have it on my ToDo list, to make this more robust.


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

I have a fix, that I will send today.