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

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?

[ ... ]

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

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

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