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.

quoted

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.

quoted

@@ -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()")

quoted

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

[ ... ]