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

On 06.08.26 18:06, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del()
> 
> This moves the kfree() of dibs->dmb_clientid_arr out of dibs_dev_del() and
> into dibs_dev_release(), so the array survives until the last device
> reference is dropped.  The error paths in dibs_dev_add() are converted from
> gotos to direct returns and no longer free the array.
> 
>> diff --git a/drivers/dibs/dibs_main.c b/drivers/dibs/dibs_main.c
>> index 14c3e2d84902e..f059fefb09bf8 100644
>> --- 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);
>>  	kfree(dibs);
>>  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the new ownership rule
> here leave a double free of dibs->dmb_clientid_arr in the loopback probe
> path?
> 
[..]


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


> 
> Since this is tagged for stable, would it be worth also stating the new
> caller contract in the changelog, i.e. that callers must only call
> put_device() when dibs_dev_add() fails?
> 

I don't understand the proposal.


>> @@ -194,12 +195,13 @@ int dibs_dev_add(struct dibs_dev *dibs)
>>  
>>  	ret = device_add(&dibs->dev);
>>  	if (ret)
>> -		goto free_client_arr;
>> +		return ret;
>>  
>>  	ret = sysfs_create_group(&dibs->dev.kobj, &dibs_dev_attr_group);
>>  	if (ret) {
>>  		dev_err(&dibs->dev, "sysfs_create_group failed for dibs_dev\n");
>> -		goto err_device_del;
>> +		device_del(&dibs->dev);
>> +		return ret;
>>  	}
> 
> [Severity: High]
> These are the two failure returns that reach the loopback err_reg path
> described above: device_add() returning an error (-ENOMEM, or a
> kobject_add()/duplicate-name failure) and sysfs_create_group() returning
> -ENOMEM.  In both cases dmb_clientid_arr is still allocated and not NULLed,
> so the caller-side kfree() and dibs_dev_release() both free it.  The
> kzalloc-failure case is fine because the pointer is still NULL.
> 

Same as above (?)

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



> [Severity: Medium]
> This is a pre-existing issue and not introduced by the patch, but the
> changelog reasons about the placement of the allocation:
> 
>     Note that allocating in dibs_dev_add() instead of dibs_dev_alloc() is ok
>     for now, because no dmbs can be registered before dibs_dev_add().
> 
> Does that reasoning also cover dibs->lock, which is initialized in the same
> place?
> 
> drivers/dibs/dibs_main.c:dibs_dev_add() {
> 	max_dmbs = dibs->ops->max_dmbs();
> 	spin_lock_init(&dibs->lock);
> 	dibs->dmb_clientid_arr = kzalloc(max_dmbs, GFP_KERNEL);
> 	...
> }
> 
> ism_probe() enables interrupts before that, via ism_dev_init() doing
> request_irq(pci_irq_vector(pdev, 0), ism_handle_irq, ...) plus
> register_sba()/register_ieq(), and only calls dibs_dev_add() afterwards.  In
> that window dibs comes from kzalloc, so dibs->lock has never been
> initialized and dmb_clientid_arr is NULL, yet the handler takes the lock and
> indexes the array before any check:
> 
> drivers/s390/net/ism_drv.c:ism_handle_irq() {
> 	spin_lock(&dibs->lock);
> 	ism->sba->s = 0;
> 	...
> 		client_id = dibs->dmb_clientid_arr[bit];
> 		if (unlikely(client_id == NO_DIBS_CLIENT ||
> 			     !dibs->subs[client_id]))
> 			continue;
> 	...
> }
> 
> The comment in ism_probe() says interrupts and events are ignored before
> dibs_dev_add() adds the device to any clients, but that check happens on
> dibs->subs[], after the lock acquisition and after the dmb_clientid_arr
> dereference.  With CONFIG_DEBUG_SPINLOCK or lockdep this window would give
> "spinlock bad magic" or "trying to register non-static key" from hardirq
> context, and spin_lock_init() in dibs_dev_add() re-zeroing the lock while
> the handler may hold it would void mutual exclusion.
> 
> Would moving spin_lock_init() and the allocation into dibs_dev_alloc() close
> this window?
> 

Recently fixed by:
c27e36054537 ("dibs: initialise dibs->lock in dibs_dev_alloc()")



>> @@ -214,13 +216,6 @@ int dibs_dev_add(struct dibs_dev *dibs)
>>  	mutex_unlock(&dibs_dev_list.mutex);
>>  
>>  	return 0;
>> -
>> -err_device_del:
>> -	device_del(&dibs->dev);
>> -free_client_arr:
>> -	kfree(dibs->dmb_clientid_arr);
>> -	return ret;
>> -
>>  }
>>  EXPORT_SYMBOL_GPL(dibs_dev_add);
> 
> [ ... ]