[PATCH net] dibs: fix double free of dmb_clientid_arr

[PATCH net] dibs: fix double free of dmb_clientid_arr

[Hidayath Khan]

dibs_dev_add() frees dibs->dmb_clientid_arr on both of its failure
paths, but does not clear the pointer. dibs_lo_dev_probe() then frees
the same pointer again in its err_reg path, so a failing device_add()
or sysfs_create_group() results in a double free.

Drop the kfree() from dibs_lo_dev_probe().

Note: commit cc21191b584c ("dibs: Move data path to dibs layer") moved
the code to its current location; the race was introduced earlier by
commit c3a910f2380f ("net/smc: implement DMB-merged operations of
loopback-ism").

Fixes: c3a910f2380f ("net/smc: implement DMB-merged operations of loopback-ism")
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_loopback.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
index fd5caf1e19a8..649e4e375be3 100644
--- a/drivers/dibs/dibs_loopback.c
+++ b/drivers/dibs/dibs_loopback.c
@@ -335,7 +335,6 @@ static int dibs_lo_dev_probe(void)
 	return 0;
 
 err_reg:
-	kfree(dibs->dmb_clientid_arr);
 	/* pairs with dibs_dev_alloc() */
 	put_device(&dibs->dev);
 	kfree(ldev);

base-commit: 98379cf6109c0adb5879f86ba3efd4afe389a64a
prerequisite-patch-id: da145e40ff1ce22e9701dd49676606f612c03872
-- 
2.52.0

Re: [PATCH net] dibs: fix double free of dmb_clientid_arr

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260730075624.114778-1-hidayath@linux.ibm.com?part=1
---
--- Patch [1]: [PATCH net] dibs: fix double free of dmb_clientid_arr ---
commit d9da866ee3cff93c660daf037f85b099a2326866
Author: Hidayath Khan <hidayath@linux.ibm.com>

dibs: fix double free of dmb_clientid_arr

This drops the kfree(dibs->dmb_clientid_arr) from the err_reg path of
dibs_lo_dev_probe(), since dibs_dev_add() already frees that buffer on
both of its own failure paths without clearing the pointer.

Two questions below, one about the commit message tags and one about the
lifetime of the same buffer on the removal path.

> dibs: fix double free of dmb_clientid_arr
>
> dibs_dev_add() frees dibs->dmb_clientid_arr on both of its failure
> paths, but does not clear the pointer. dibs_lo_dev_probe() then frees
> the same pointer again in its err_reg path, so a failing device_add()
> or sysfs_create_group() results in a double free.
>
> Drop the kfree() from dibs_lo_dev_probe().
>
> Note: commit cc21191b584c ("dibs: Move data path to dibs layer") moved
> the code to its current location; the race was introduced earlier by
> commit c3a910f2380f ("net/smc: implement DMB-merged operations of
> loopback-ism").
>
> Fixes: c3a910f2380f ("net/smc: implement DMB-merged operations of loopback-ism")
> Cc: stable@vger.kernel.org

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
Is c3a910f2380f the right commit to blame here?

That commit only touches net/smc/smc_loopback.c and net/smc/smc_loopback.h,
adding DMB refcounting plus attach/detach/nocopy support. It creates no
client-id array, and it adds no kfree() on any probe error path. At
c3a910f2380f^ the only matching array in the tree is ISM's private
sba_client_arr in drivers/s390/net/ism_drv.c and include/linux/ism.h.

Blame instead points both halves of the double free at cc21191b584c
("dibs: Move data path to dibs layer"): the allocation and the free in
dibs_dev_add(),

	dibs->dmb_clientid_arr = kzalloc(max_dmbs, GFP_KERNEL);
	...
	free_client_arr:
		kfree(dibs->dmb_clientid_arr);

and the duplicate kfree() in dibs_lo_dev_probe()'s err_reg path that this
patch removes. Would it be more accurate to say cc21191b584c introduced the
problem, rather than that it "moved the code to its current location; the
race was introduced earlier"?

With Cc: stable@vger.kernel.org present, a Fixes tag naming a v6.10-era
net/smc commit points backports at trees where neither drivers/dibs/ nor
dmb_clientid_arr exists, so the patch cannot apply there.

Also, the note calls this a "race", but the double free happens
unconditionally on a deterministic error path when device_add() or
sysfs_create_group() fails. Would "double free" be the better wording?

> diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
> index fd5caf1e19a8f..649e4e375be33 100644
> --- a/drivers/dibs/dibs_loopback.c
> +++ b/drivers/dibs/dibs_loopback.c
> @@ -335,7 +335,6 @@ static int dibs_lo_dev_probe(void)
>  	return 0;
>  
>  err_reg:
> -	kfree(dibs->dmb_clientid_arr);
>  	/* pairs with dibs_dev_alloc() */
>  	put_device(&dibs->dev);
>  	kfree(ldev);

[Severity: High]
[Finding: main-5-4]
[Sources: opus-5]
This isn't a bug introduced by this patch, but while looking at the
lifetime of dmb_clientid_arr: does the removal path have the same missing
NULL assignment, and can the ISM interrupt handler read the buffer after it
is freed?

dibs_dev_del() in drivers/dibs/dibs_main.c ends with the free outside
dibs->lock and leaves the pointer set:

	device_del(&dibs->dev);
	kfree(dibs->dmb_clientid_arr);

The spin_lock_irqsave(&dibs->lock) section earlier in that function only
clears dibs->subs[], and is released well before the kfree().

ism_remove() in drivers/s390/net/ism_drv.c then runs:

	dibs_dev_del(dibs);
	ism_dev_exit(ism);

and ism_dev_exit() is the only place that calls unregister_sba() and
free_irq(pci_irq_vector(pdev, 0), ism). So the SBA stays registered and the
MSI handler stays armed across the kfree(). ism_handle_irq() does, for every
set DMB bit while holding dibs->lock:

	client_id = dibs->dmb_clientid_arr[bit];
	if (unlikely(client_id == NO_DIBS_CLIENT ||
		     !dibs->subs[client_id]))
		continue;
	dibs->subs[client_id]->ops->handle_irq(dibs, ...);

Since the free does not take dibs->lock, can the handler be mid-loop while
the removal path frees the array underneath it?

There is a second effect if the byte read back is garbage: only
NO_DIBS_CLIENT is rejected, while subs[] has MAX_DIBS_CLIENTS == 8 entries
and is the last member of struct dibs_dev in include/linux/dibs.h. Can a
client_id in 8..254 read past the end of the kmalloc'ed struct dibs_dev and,
if that slot is non-NULL, produce an indirect call through a bogus struct
dibs_client pointer?

For the window to be reachable a DMB bit must still be settable by the peer.
ism_unregister_dmb() writes NO_DIBS_CLIENT and then issues the hardware
unregister command, but:

	ret = ism_cmd(ism, &cmd);
	if (ret && ret != ISM_ERROR)
		goto out;

leaves the DMB registered in the adapter when the command fails, so a remote
peer can keep signalling it.

Would it make sense to free and NULL dmb_clientid_arr only after the owning
driver has quiesced its interrupt and data path, do it under dibs->lock, and
bound-check client_id against MAX_DIBS_CLIENTS in ism_handle_irq()? The
missing NULL assignment in dibs_dev_add() is also what turned the removed
kfree() into a double free.

Re: [PATCH net] dibs: fix double free of dmb_clientid_arr

[Alexandra Winter]

On 04.08.26 18:52, Simon Horman 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.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260730075624.114778-1-hidayath@linux.ibm.com?part=1
> ---
> --- Patch [1]: [PATCH net] dibs: fix double free of dmb_clientid_arr ---
> commit d9da866ee3cff93c660daf037f85b099a2326866
> Author: Hidayath Khan <hidayath@linux.ibm.com>
> 
> dibs: fix double free of dmb_clientid_arr
> 
> This drops the kfree(dibs->dmb_clientid_arr) from the err_reg path of
> dibs_lo_dev_probe(), since dibs_dev_add() already frees that buffer on
> both of its own failure paths without clearing the pointer.
> 
> Two questions below, one about the commit message tags and one about the
> lifetime of the same buffer on the removal path.
> 
>> dibs: fix double free of dmb_clientid_arr
>>
>> dibs_dev_add() frees dibs->dmb_clientid_arr on both of its failure
>> paths, but does not clear the pointer. dibs_lo_dev_probe() then frees
>> the same pointer again in its err_reg path, so a failing device_add()
>> or sysfs_create_group() results in a double free.
>>
>> Drop the kfree() from dibs_lo_dev_probe().
>>
>> Note: commit cc21191b584c ("dibs: Move data path to dibs layer") moved
>> the code to its current location; the race was introduced earlier by
>> commit c3a910f2380f ("net/smc: implement DMB-merged operations of
>> loopback-ism").
>>
>> Fixes: c3a910f2380f ("net/smc: implement DMB-merged operations of loopback-ism")
>> Cc: stable@vger.kernel.org
> 
> [Severity: Medium]
> [Finding: main-1-0]
> [Sources: opus-5]
> Is c3a910f2380f the right commit to blame here?
> 
> That commit only touches net/smc/smc_loopback.c and net/smc/smc_loopback.h,
> adding DMB refcounting plus attach/detach/nocopy support. It creates no
> client-id array, and it adds no kfree() on any probe error path. At
> c3a910f2380f^ the only matching array in the tree is ISM's private
> sba_client_arr in drivers/s390/net/ism_drv.c and include/linux/ism.h.
> 
> Blame instead points both halves of the double free at cc21191b584c
> ("dibs: Move data path to dibs layer"): the allocation and the free in
> dibs_dev_add(),
> 
> 	dibs->dmb_clientid_arr = kzalloc(max_dmbs, GFP_KERNEL);
> 	...
> 	free_client_arr:
> 		kfree(dibs->dmb_clientid_arr);
> 
> and the duplicate kfree() in dibs_lo_dev_probe()'s err_reg path that this
> patch removes. Would it be more accurate to say cc21191b584c introduced the
> problem, rather than that it "moved the code to its current location; the
> race was introduced earlier"?
> 

Hidayath,
I think, AI is right here, the original smc_loopback.c did not have such a construct.
Only cc21191b584c is to blame.
(Seems you errounously copied this from your other patch
a10ea943356b ("dibs: fix use-after-free of dmb_node in loopback attach/detach/unregister")
Please send a v2



> With Cc: stable@vger.kernel.org present, a Fixes tag naming a v6.10-era
> net/smc commit points backports at trees where neither drivers/dibs/ nor
> dmb_clientid_arr exists, so the patch cannot apply there.
> 
> Also, the note calls this a "race", but the double free happens
> unconditionally on a deterministic error path when device_add() or
> sysfs_create_group() fails. Would "double free" be the better wording?
> 


Correct (but moot, if this paragraph is removed)


>> diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
>> index fd5caf1e19a8f..649e4e375be33 100644
>> --- a/drivers/dibs/dibs_loopback.c
>> +++ b/drivers/dibs/dibs_loopback.c
>> @@ -335,7 +335,6 @@ static int dibs_lo_dev_probe(void)
>>  	return 0;
>>  
>>  err_reg:
>> -	kfree(dibs->dmb_clientid_arr);
>>  	/* pairs with dibs_dev_alloc() */
>>  	put_device(&dibs->dev);
>>  	kfree(ldev);
> 
> [Severity: High]
> [Finding: main-5-4]
> [Sources: opus-5]
> This isn't a bug introduced by this patch, but while looking at the
> lifetime of dmb_clientid_arr: does the removal path have the same missing
> NULL assignment, and can the ISM interrupt handler read the buffer after it
> is freed?
> 


[PATCH net] net/dibs: Fix UAF of dmb_clientid_arr after dibs_dev_del()
is currently discussed on the netdev mailing list to fix this.

[PATCH net v2] dibs: fix double free of dmb_clientid_arr

[Hidayath Khan]

dibs_dev_add() frees dibs->dmb_clientid_arr on both of its failure
paths, but does not clear the pointer. dibs_lo_dev_probe() then frees
the same pointer again in its err_reg path, so a failing device_add()
or sysfs_create_group() results in a double free.

Drop the kfree() from dibs_lo_dev_probe().

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>
---
v2:
- Fix Fixes: tag (was c3a910f2380f, should be cc21191b584c)
- Remove incorrect Note paragraph copied from a different patch

 drivers/dibs/dibs_loopback.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/dibs/dibs_loopback.c b/drivers/dibs/dibs_loopback.c
index fd5caf1e19a8..649e4e375be3 100644
--- a/drivers/dibs/dibs_loopback.c
+++ b/drivers/dibs/dibs_loopback.c
@@ -335,7 +335,6 @@ static int dibs_lo_dev_probe(void)
 	return 0;
 
 err_reg:
-	kfree(dibs->dmb_clientid_arr);
 	/* pairs with dibs_dev_alloc() */
 	put_device(&dibs->dev);
 	kfree(ldev);

base-commit: 92a09f2af870e91efb9471b109db15c2030f90f2
-- 
2.52.0