linux-kernel - Re: [PATCH net] net: hns3: fix double free issue for tx spare buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <81f552d6-fc53-4aa0-8c7d-d098a3bb6f04@huawei.com>
Date: Wed, 4 Feb 2026 22:25:52 +0800
From: Jijie Shao <shaojijie@...wei.com>
To: Jacob Keller <jacob.e.keller@...el.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<andrew+netdev@...n.ch>, <horms@...nel.org>
CC: <shaojijie@...wei.com>, <shenjian15@...wei.com>, <liuyonglong@...wei.com>,
	<chenhao418@...wei.com>, <lantao5@...wei.com>,
	<huangdonghua3@...artners.com>, <yangshuaisong@...artners.com>,
	<jonathan.cameron@...wei.com>, <salil.mehta@...wei.com>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: hns3: fix double free issue for tx spare buffer


on 2026/2/4 9:30, Jacob Keller wrote:
>
>
> On 2/2/2026 2:58 AM, Jijie Shao wrote:
>> From: Jian Shen <shenjian15@...wei.com>
>>
>> The driver missed to clear ring->tx_spare to NULL when
>> fail to initialize tx spare buffer. And it will try to
>> free the tx spare buffer in hns3_fini_ring() if tx_spare
>> is not NULL. So it may cause double free issue.
>>
>> Fixes: 907676b130711 ("net: hns3: use tx bounce buffer for small 
>> packets")
>> Signed-off-by: Jian Shen <shenjian15@...wei.com>
>> Signed-off-by: Jijie Shao <shaojijie@...wei.com>
>> ---
>>   drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c 
>> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> index 7a9573dcab74..e879b04e21b0 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> @@ -1048,13 +1048,13 @@ static void hns3_init_tx_spare_buffer(struct 
>> hns3_enet_ring *ring)
>>       int order;
>>         if (!alloc_size)
>> -        return;
>> +        goto not_init;
>>         order = get_order(alloc_size);
>>       if (order > MAX_PAGE_ORDER) {
>>           if (net_ratelimit())
>>               dev_warn(ring_to_dev(ring), "failed to allocate tx 
>> spare buffer, exceed to max order\n");
>> -        return;
>> +        goto not_init;
>>       }
>>         tx_spare = devm_kzalloc(ring_to_dev(ring), sizeof(*tx_spare),
>> @@ -1092,6 +1092,13 @@ static void hns3_init_tx_spare_buffer(struct 
>> hns3_enet_ring *ring)
>>       devm_kfree(ring_to_dev(ring), tx_spare);
>
>
>>   devm_kzalloc_error:
>>       ring->tqp->handle->kinfo.tx_spare_buf_size = 0;
>> +not_init:
>> +    /* When driver init or reset_init, the ring->tx_spare is always 
>> NULL;
>> +     * but when called from hns3_set_ringparam, it's usually not 
>> NULL, and
>> +     * will be restored if hns3_init_all_ring() failed. So it's safe 
>> to set
>> +     * ring->tx_spare to NULL here.
>> +     */
>> +    ring->tx_spare = NULL;
>
> This feels weird. If it was already NULL there's no reason to write a 
> NULL to it. And if it isn't NULL, we should have released and 
> overwritten it previously by whatever caller was going to free it, 
> otherwise you'd just have a different kind of memory leak...
>
> It looks like the only places that call devm_kfree on the tx_spare is 
> hns3_fini_ring. It sets the value to NULL after freeing it. I am not 
> following how it can ever be non-NULL where setting this to NULL and 
> overwriting it would not lead to a memory leak (aside from the fact 
> that this is devm memory and will be released on removing the device)
>
> Is it because the ring memory isn't zero-initialized?? But that 
> wouldn't be a double-free that would be a freeing a bad address...
>
> Hmm. You create a copy of the ring parameters in hns3_set_ringparam. 
> Then if that fails, you copy things back. Otherwise you release the 
> tmp_rings... but the new rings didn't get cleared memory.. So the 
> issue is that your ring structures effectively have garbage memory in 
> them when initialized in set_ringparam.
>
> You can't just check the old tx_spare and release it here because 
> doing so would corrupt the memory pointed by the temporary copy made 
> when changing ring paramters (as it would no longer be valid), and you 
> can't assume it was zero initialized because your new rings were built 
> in place on top of the old ring structures.
>
> Ok. The fix makes sense but it does take a bit to understand why. You 
> effectively have to treat the memory in tx_spare as "uninitialized 
> garbage".
>
> Other solutions would require more significant refactoring. You could 
> memset or otherwise zero out the ring array structures entirely when 
> allocating new rings in hns3_init_all_ring() for example. Not sure 
> that is any easier to follow, and its not my driver so:
>
> Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

Thank you for your careful review, I apologize for the trouble this patch has caused you.

We considered the logic to be quite complex, so we added comments in the code to explain it.
However, for reviewers who are not familiar with our driver, these comments might be a bit unclear.
I will add more detailed explanations in the commit message for v2.

Thanks,
Jijie Shao


>
>>   }
>>     /* Use hns3_tx_spare_space() to make sure there is enough buffer
>
>