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@xxxxxxxxxxxxx?part=1
> ---
> --- Patch [1]: [PATCH net] dibs: fix double free of dmb_clientid_arr ---
> commit d9da866ee3cff93c660daf037f85b099a2326866
> Author: Hidayath Khan <hidayath@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> 
> [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@xxxxxxxxxxxxxxx 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.