Re: [PATCH net] dibs: initialise dibs->lock in dibs_dev_alloc()

On 31.07.26 14:42, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Early interrupt handling causes NULL pointer dereferences on `ism->sba` and `dibs->dmb_clientid_arr`.
> - [High] The interrupt handler is unregistered (`free_irq`) after the critical data structures it accesses are freed, leading to Use-After-Free or NULL pointer dereferences during teardown.
> - [High] Double free of `dibs->dmb_clientid_arr` when `dibs_dev_add` fails.
> --
> 
> --- Patch [1]: [PATCH net] dibs: initialise dibs->lock in dibs_dev_alloc() ---
> commit dbb6bc52f9b974f5e291d1236762d41302672810
> Author: Hidayath Khan <[email protected]>
> 
> dibs: initialise dibs->lock in dibs_dev_alloc()
> 
> dibs->lock is initialised by dibs_dev_add(), but a dibs device can already
> take interrupts before that call. This moves the lock initialization to
> dibs_dev_alloc() so it is valid as soon as the device can be published to
> its interrupt handler.
> 
>> dibs->lock is initialised by dibs_dev_add(), but a dibs device can
>> already take interrupts before that call: ism_probe() runs
>> ism_dev_init(), and hence request_irq(), before it calls
>> dibs_dev_add(). No client can have registered a dmb at that point, so
>> no dmb interrupt can occur, but a GID event interrupt can, and
>> ism_handle_irq() takes dibs->lock unconditionally on entry, before it
>> inspects anything else.
> 
> [Severity: High]
> This is a pre-existing issue, but while moving the lock initialization avoids a
> panic on the uninitialized spinlock, won't the handler still crash immediately
> after taking the lock?
> 
> Looking at drivers/s390/net/ism_drv.c, if a spurious interrupt fires right after
> request_irq() in 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);
> 
> The interrupt handler ism_handle_irq() will execute and unconditionally
> dereference ism->sba, which hasn't been populated by register_sba() yet:
> 
> 	bv = (void *) &ism->sba->dmb_bits[ISM_DMB_WORD_OFFSET];
> 	end = sizeof(ism->sba->dmb_bits) * BITS_PER_BYTE - ISM_DMB_BIT_OFFSET;
> 
> 	spin_lock(&dibs->lock);
> 	ism->sba->s = 0;
> 
> Could this cause a NULL pointer dereference when accessing ism->sba?
> 
> [ ... ]

ISM devices will only deliver interrupts when sba AND ieq are registered.
I will add a comment.



> 
>> diff --git a/drivers/dibs/dibs_main.c b/drivers/dibs/dibs_main.c
>> index 14c3e2d84902e..4c26fd06973f7 100644
>> --- a/drivers/dibs/dibs_main.c
>> +++ b/drivers/dibs/dibs_main.c
> 
> [ ... ]
> 
>> @@ -186,7 +187,6 @@ int dibs_dev_add(struct dibs_dev *dibs)
>>  	int i, ret;
>>  
>>  	max_dmbs = dibs->ops->max_dmbs();
>> -	spin_lock_init(&dibs->lock);
>>  	dibs->dmb_clientid_arr = kzalloc(max_dmbs, GFP_KERNEL);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this allocation lead to a
> double free on error paths?
> 
> If dibs_dev_add() fails later in the function, it legitimately frees this
> array:
> 
> drivers/dibs/dibs_main.c:dibs_dev_add() {
>     ...
> free_client_arr:
> 	kfree(dibs->dmb_clientid_arr);
> 	return ret;
> }
> 
> However, the loopback driver's probe function dibs_lo_dev_probe() also frees it
> on failure:
> 
> 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);
> }
> 
> Can this cause a double free of dibs->dmb_clientid_arr if device registration or
> sysfs group creation fails?


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


> 
> [Severity: High]
> This is a pre-existing issue, but is there also a use-after-free risk during
> device teardown involving this array?
> 
> During device removal, ism_remove() executes teardown in an order that frees
> data structures while the interrupt handler is still active:
> 
> drivers/s390/net/ism_drv.c:ism_remove() {
> 	...
> 	dibs_dev_del(dibs);
> 	ism_dev_exit(ism);
> 	...
> }
> 
> dibs_dev_del() calls kfree(dibs->dmb_clientid_arr):
> 
> drivers/dibs/dibs_main.c:dibs_dev_del() {
> 	...
> 	device_del(&dibs->dev);
> 	kfree(dibs->dmb_clientid_arr);
> }
> 
> And then ism_dev_exit() unregisters and nullifies ism->sba before finally
> calling free_irq():
> 
> drivers/s390/net/ism_drv.c:ism_dev_exit() {
> 	...
> 	unregister_sba(ism);
> 	free_irq(pci_irq_vector(pdev, 0), ism);
> }
> 
> If an interrupt fires during this window, can the active IRQ handler access the
> freed dibs->dmb_clientid_arr or dereference the NULL ism->sba pointer?
> 
>>  	if (!dibs->dmb_clientid_arr)
>>  		return -ENOMEM;
> 

I will send fixes.