* [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del()
@ 2026-08-04 8:58 Alexandra Winter
2026-08-05 8:59 ` sashiko-bot
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Alexandra Winter @ 2026-08-04 8:58 UTC (permalink / raw)
To: Hidayath Khan, David Miller, Jakub Kicinski, Paolo Abeni,
Eric Dumazet, Andrew Lunn
Cc: netdev, linux-s390, linux-kernel, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Simon Horman, stable
A dibs device interrupt handler can be active after dibs_dev_del() and
may still access dmb_clientid_arr.
Free dmb_clientid_arr in dibs_dev_release() after last reference is gone.
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().
Fixes: cc21191b584c ("dibs: Move data path to dibs layer")
Cc: stable@vger.kernel.org
Co-developed-by: Hidayath Khan <hidayath@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
---
drivers/dibs/dibs_main.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/dibs/dibs_main.c b/drivers/dibs/dibs_main.c
index 14c3e2d84902..f059fefb09bf 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);
}
@@ -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;
}
mutex_lock(&dibs_dev_list.mutex);
mutex_lock(&clients_lock);
@@ -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);
@@ -247,7 +242,6 @@ void dibs_dev_del(struct dibs_dev *dibs)
mutex_unlock(&dibs_dev_list.mutex);
device_del(&dibs->dev);
- kfree(dibs->dmb_clientid_arr);
}
EXPORT_SYMBOL_GPL(dibs_dev_del);
--
2.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() 2026-08-04 8:58 [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() Alexandra Winter @ 2026-08-05 8:59 ` sashiko-bot 2026-08-05 12:14 ` Alexandra Winter 2026-08-06 13:02 ` Simon Horman 2026-08-06 16:06 ` Jakub Kicinski 2 siblings, 1 reply; 7+ messages in thread From: sashiko-bot @ 2026-08-05 8:59 UTC (permalink / raw) To: Alexandra Winter Cc: Alexander Gordeev, linux-s390, Vasily Gorbik, Heiko Carstens Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: Pre-existing issues: - [Critical] Double free of `dmb_clientid_arr` in the error path of `dibs_lo_dev_probe()`. - [Critical] The interrupt handler `ism_handle_irq()` can dereference NULL pointers and acquire an uninitialized lock due to early interrupt enablement. - [Critical] Use-after-free of `ism->sba` and `ism->ieq` during teardown due to improper ordering with `free_irq()`. - [High] Partial rollback failure leaves client in a broken state in `dibs_unregister_client()`. -- --- Patch [1]: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() --- commit 40d2232069f48423978af98bbecb5075fb611ca0 Author: Alexandra Winter <wintera@linux.ibm.com> net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() A dibs device interrupt handler can be active after dibs_dev_del() and may still access dmb_clientid_arr. Free dmb_clientid_arr in dibs_dev_release() after last reference is gone. Fixes: cc21191b584c ("dibs: Move data path to dibs layer") Cc: stable@vger.kernel.org Co-developed-by: Hidayath Khan <hidayath@linux.ibm.com> Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > --- 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); [Severity: Critical] This is a pre-existing issue, but does this lead to a double free in the error path of dibs_lo_dev_probe()? If device registration fails during loopback probe, the error path explicitly frees the array and then puts the device: 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); ... } Since put_device() now unconditionally calls dibs_dev_release() and frees the array again, will this corrupt memory? [ ... ] > @@ -194,12 +195,13 @@ int dibs_dev_add(struct dibs_dev *dibs) [Severity: Critical] This isn't a bug introduced by this patch, but can the interrupt handler access uninitialized structures if an early interrupt occurs? In ism_dev_init(), interrupts are requested and enabled before the device is fully registered: drivers/s390/net/ism_drv.c: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); ... } If a hardware interrupt arrives immediately after request_irq(), ism_handle_irq() will execute and unconditionally access the uninitialized ism->sba and acquire dibs->lock, before register_sba() or dibs_dev_add() have initialized them. Can this lead to a NULL pointer dereference or uninitialized lock access? [ ... ] [Severity: High] This is a pre-existing issue, but does dibs_unregister_client() fail to roll back its state if a client is busy? If a client attempts to unregister while still having active DMBs on multiple devices, the loop aborts and returns -EBUSY: drivers/dibs/dibs_main.c:dibs_unregister_client() { ... rc = -EBUSY; goto err_reg_dmb; } } /* Stop forwarding IRQs and events */ dibs->subs[client->id] = NULL; spin_unlock_irqrestore(&dibs->lock, flags); clients[client->id]->ops->del_dev(dibs); ... } Since it does not undo the modifications made to the preceding devices in the list (such as clearing dibs->subs[client->id]), will this leave the client in a broken state? [Severity: Critical] This isn't a bug introduced by this patch, but can a device interrupt cause a use-after-free during driver removal? In ism_dev_exit(), the data structures are destroyed before the interrupt handler is quiesced: drivers/s390/net/ism_drv.c:ism_dev_exit() { struct pci_dev *pdev = ism->pdev; unregister_ieq(ism); unregister_sba(ism); free_irq(pci_irq_vector(pdev, 0), ism); ... } If an interrupt arrives after unregister_sba() but before free_irq(), ism_handle_irq() will access the freed ism->sba. Should free_irq() be called before tearing down the data structures? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260804085848.3579518-1-wintera@linux.ibm.com?part=1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() 2026-08-05 8:59 ` sashiko-bot @ 2026-08-05 12:14 ` Alexandra Winter 0 siblings, 0 replies; 7+ messages in thread From: Alexandra Winter @ 2026-08-05 12:14 UTC (permalink / raw) To: sashiko-reviews Cc: Alexander Gordeev, linux-s390, Vasily Gorbik, Heiko Carstens On 05.08.26 10:59, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: > > Pre-existing issues: > - [Critical] Double free of `dmb_clientid_arr` in the error path of `dibs_lo_dev_probe()`. > - [Critical] The interrupt handler `ism_handle_irq()` can dereference NULL pointers and acquire an uninitialized lock due to early interrupt enablement. > - [Critical] Use-after-free of `ism->sba` and `ism->ieq` during teardown due to improper ordering with `free_irq()`. > - [High] Partial rollback failure leaves client in a broken state in `dibs_unregister_client()`. > -- > > --- Patch [1]: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() --- > commit 40d2232069f48423978af98bbecb5075fb611ca0 > Author: Alexandra Winter <wintera@linux.ibm.com> > > net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() > > A dibs device interrupt handler can be active after dibs_dev_del() and > may still access dmb_clientid_arr. Free dmb_clientid_arr in > dibs_dev_release() after last reference is gone. > > Fixes: cc21191b584c ("dibs: Move data path to dibs layer") > Cc: stable@vger.kernel.org > Co-developed-by: Hidayath Khan <hidayath@linux.ibm.com> > Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com> > Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > >> --- 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); > > [Severity: Critical] > This is a pre-existing issue, but does this lead to a double free in the > error path of dibs_lo_dev_probe()? > > If device registration fails during loopback probe, the error path explicitly > frees the array and then puts the device: > > 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); > ... > } > > Since put_device() now unconditionally calls dibs_dev_release() and frees > the array again, will this corrupt memory? > > [ ... ] > [PATCH net] dibs: fix double free of dmb_clientid_arr is in discussion on the netdev mailinglist already. >> @@ -194,12 +195,13 @@ int dibs_dev_add(struct dibs_dev *dibs) > > [Severity: Critical] > This isn't a bug introduced by this patch, but can the interrupt handler > access uninitialized structures if an early interrupt occurs? > > In ism_dev_init(), interrupts are requested and enabled before the device > is fully registered: > > drivers/s390/net/ism_drv.c: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); > ... > } > > If a hardware interrupt arrives immediately after request_irq(), > ism_handle_irq() will execute and unconditionally access the uninitialized > ism->sba and acquire dibs->lock, before register_sba() or dibs_dev_add() > have initialized them. Can this lead to a NULL pointer dereference or > uninitialized lock access? > > [ ... ] Cannot happen. ism hardware will not generate interrupts before SBA AND IEQ are registered. A cleanup patch is planned. > > [Severity: High] > This is a pre-existing issue, but does dibs_unregister_client() fail to > roll back its state if a client is busy? > > If a client attempts to unregister while still having active DMBs on > multiple devices, the loop aborts and returns -EBUSY: > > drivers/dibs/dibs_main.c:dibs_unregister_client() { > ... > rc = -EBUSY; > goto err_reg_dmb; > } > } > /* Stop forwarding IRQs and events */ > dibs->subs[client->id] = NULL; > spin_unlock_irqrestore(&dibs->lock, flags); > clients[client->id]->ops->del_dev(dibs); > ... > } > > Since it does not undo the modifications made to the preceding devices in > the list (such as clearing dibs->subs[client->id]), will this leave the > client in a broken state? > There is no such codepath with today's SMC client. I have it on my ToDo list, to make this more robust. > > [Severity: Critical] > This isn't a bug introduced by this patch, but can a device interrupt cause > a use-after-free during driver removal? > > In ism_dev_exit(), the data structures are destroyed before the interrupt > handler is quiesced: > > drivers/s390/net/ism_drv.c:ism_dev_exit() { > struct pci_dev *pdev = ism->pdev; > > unregister_ieq(ism); > unregister_sba(ism); > free_irq(pci_irq_vector(pdev, 0), ism); > ... > } > > If an interrupt arrives after unregister_sba() but before free_irq(), > ism_handle_irq() will access the freed ism->sba. Should free_irq() be > called before tearing down the data structures? > I have a fix, that I will send today. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() 2026-08-04 8:58 [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() Alexandra Winter 2026-08-05 8:59 ` sashiko-bot @ 2026-08-06 13:02 ` Simon Horman 2026-08-06 16:06 ` Jakub Kicinski 2 siblings, 0 replies; 7+ messages in thread From: Simon Horman @ 2026-08-06 13:02 UTC (permalink / raw) To: Alexandra Winter Cc: Hidayath Khan, David Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, Andrew Lunn, netdev, linux-s390, linux-kernel, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Christian Borntraeger, Sven Schnelle, stable On Tue, Aug 04, 2026 at 10:58:48AM +0200, Alexandra Winter wrote: > A dibs device interrupt handler can be active after dibs_dev_del() and > may still access dmb_clientid_arr. > > Free dmb_clientid_arr in dibs_dev_release() after last reference is gone. > 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(). > > Fixes: cc21191b584c ("dibs: Move data path to dibs layer") > Cc: stable@vger.kernel.org > Co-developed-by: Hidayath Khan <hidayath@linux.ibm.com> > Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com> > Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> Reviewed-by: Simon Horman <horms@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() 2026-08-04 8:58 [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() Alexandra Winter 2026-08-05 8:59 ` sashiko-bot 2026-08-06 13:02 ` Simon Horman @ 2026-08-06 16:06 ` Jakub Kicinski 2026-08-07 14:07 ` Alexandra Winter 2 siblings, 1 reply; 7+ messages in thread From: Jakub Kicinski @ 2026-08-06 16:06 UTC (permalink / raw) To: wintera Cc: Jakub Kicinski, hidayath, davem, pabeni, edumazet, andrew+netdev, netdev, linux-s390, linux-kernel, hca, gor, agordeev, borntraeger, svens, horms, stable 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? After this change dibs_dev_release() is the sole owner of the buffer, while dibs_lo_dev_probe() in drivers/dibs/dibs_loopback.c still frees it itself when dibs_dev_add() fails: drivers/dibs/dibs_loopback.c:dibs_lo_dev_probe() { ... ret = dibs_dev_add(dibs); if (ret) goto err_reg; lo_dev = ldev; return 0; err_reg: kfree(dibs->dmb_clientid_arr); /* pairs with dibs_dev_alloc() */ put_device(&dibs->dev); kfree(ldev); ... } dibs_dev_alloc() leaves the refcount at 1 via device_initialize(), and the get_device()/put_device() pair inside device_add() nets to zero, so the put_device() above drops the last reference and runs dibs_dev_release(), which frees the same pointer a second time. In the baseline this was already a double free (dibs_dev_add() freed the array on its free_client_arr path and dibs_lo_dev_probe() freed it again), so the defect predates the patch, but since this patch is the one that redefines who owns dmb_clientid_arr, should the kfree() at err_reg in dibs_lo_dev_probe() be dropped in the same change? For comparison, drivers/s390/net/ism_drv.c:ism_probe() already relies only on put_device() and has no explicit kfree, so the two callers of the same exported API currently disagree about ownership: drivers/s390/net/ism_drv.c:ism_probe() { ... err_ism: ism_dev_exit(ism); err_dibs: /* pairs with dibs_dev_alloc() */ put_device(&dibs->dev); ... } 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? > @@ -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. [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? > @@ -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); [ ... ] -- pw-bot: cr ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() 2026-08-06 16:06 ` Jakub Kicinski @ 2026-08-07 14:07 ` Alexandra Winter 2026-08-07 22:00 ` Jakub Kicinski 0 siblings, 1 reply; 7+ messages in thread From: Alexandra Winter @ 2026-08-07 14:07 UTC (permalink / raw) To: Jakub Kicinski Cc: hidayath, davem, pabeni, edumazet, andrew+netdev, netdev, linux-s390, linux-kernel, hca, gor, agordeev, borntraeger, svens, horms, stable 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); > > [ ... ] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() 2026-08-07 14:07 ` Alexandra Winter @ 2026-08-07 22:00 ` Jakub Kicinski 0 siblings, 0 replies; 7+ messages in thread From: Jakub Kicinski @ 2026-08-07 22:00 UTC (permalink / raw) To: Alexandra Winter Cc: hidayath, davem, pabeni, edumazet, andrew+netdev, netdev, linux-s390, linux-kernel, hca, gor, agordeev, borntraeger, svens, horms, stable On Fri, 7 Aug 2026 16:07:53 +0200 Alexandra Winter wrote: > > [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. Please don't expect me to fish out such codependent patches magically without so much as a note in the commit msg. You have to repost this, and please do a better job of grouping the fixes going forward :/ This is a waste of time for both of us. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-08-07 22:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-04 8:58 [PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del() Alexandra Winter 2026-08-05 8:59 ` sashiko-bot 2026-08-05 12:14 ` Alexandra Winter 2026-08-06 13:02 ` Simon Horman 2026-08-06 16:06 ` Jakub Kicinski 2026-08-07 14:07 ` Alexandra Winter 2026-08-07 22:00 ` Jakub Kicinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox