Linux s390 Architecture development
 help / color / mirror / Atom feed
* [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