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

On 31.07.26 09:56, [email protected] wrote:
> 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))
> ...
> }

Fixed by
a10ea943356b ("dibs: fix use-after-free of dmb_node in loopback attach/detach/unregister")
that got accepted to net yesterday.


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

Hidayath has already sent a fix for the lock initialization:
https://lore.kernel.org/netdev/[email protected]/



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


No client can have registered a dmb at that point, so
no dmb interrupt can occur, so this NULL pointer dereference cannot happen irl.
I plan to restructure with a future cleanup patch.


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


Yes, we will send a fix.


> [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);
> 


Does not happen with smc today.
But the guard is not clean, it should do a better and safer cleanup,
should such a situation arise with a future client.
I will work on an improvement.