* 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-06 13:02 ` Simon Horman
2026-08-06 16:06 ` Jakub Kicinski
1 sibling, 0 replies; 3+ 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] 3+ 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-06 13:02 ` Simon Horman
@ 2026-08-06 16:06 ` Jakub Kicinski
1 sibling, 0 replies; 3+ 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] 3+ messages in thread