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

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

[Hidayath Khan]

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.

Initialise the lock in dibs_dev_alloc() instead, so that it is valid as
soon as a driver can publish the device to its interrupt handler.

Fixes: cc21191b584c ("dibs: Move data path to dibs layer")
Cc: stable@vger.kernel.org
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 drivers/dibs/dibs_main.c | 2 +-
 include/linux/dibs.h     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dibs/dibs_main.c b/drivers/dibs/dibs_main.c
index 14c3e2d84902..4c26fd06973f 100644
--- a/drivers/dibs/dibs_main.c
+++ b/drivers/dibs/dibs_main.c
@@ -138,6 +138,7 @@ struct dibs_dev *dibs_dev_alloc(void)
 	dibs = kzalloc_obj(*dibs);
 	if (!dibs)
 		return dibs;
+	spin_lock_init(&dibs->lock);
 	dibs->dev.release = dibs_dev_release;
 	dibs->dev.class = &dibs_class;
 	device_initialize(&dibs->dev);
@@ -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);
 	if (!dibs->dmb_clientid_arr)
 		return -ENOMEM;
diff --git a/include/linux/dibs.h b/include/linux/dibs.h
index c75607f8a5cf..d3e0777f25ae 100644
--- a/include/linux/dibs.h
+++ b/include/linux/dibs.h
@@ -439,7 +439,7 @@ static inline void *dibs_get_priv(struct dibs_dev *dev,
 /**
  * dibs_dev_alloc() - allocate and reference device structure
  *
- * The following fields will be valid upon successful return: dev
+ * The following fields will be valid upon successful return: dev, lock
  * NOTE: Use put_device(dibs_get_dev(@dibs)) to give up your reference instead
  * of freeing @dibs @dev directly once you have successfully called this
  * function.

base-commit: 10019318fb9fd64fda6ed43f9b34d0c9077d5802
-- 
2.52.0

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

[sashiko-bot]

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 <hidayath@linux.ibm.com>

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/20260730124227.167829-1-hidayath@linux.ibm.com?part=1