netdev - 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: <2b7049bd-1b85-4e6a-be3e-0f3efe3b809a@intel.com>
Date: Tue, 3 Feb 2026 17:30:45 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jijie Shao <shaojijie@...wei.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<andrew+netdev@...n.ch>, <horms@...nel.org>
CC: <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 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>

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