Re: [PATCH v2 net 3/3] net: hns3: fix use-after-free in debugfs read during reset/unload

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 1 (2) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] dbg_mutex is taken only on the teardown paths
  (hns3_reset_notify_uninit_enet, hns3_client_uninit,…
- [Medium] The Fixes tag and changelog put the whole root cause on
  eced3d1c41db ("net: hns3: use seq_file for files in queue/ in…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  rewrites the guard and makes the outcome worse.
- [Medium] This problem wasn't introduced by this patch, but
  hns3_set_ringparam() reports success for a failed ring resize when the…
- [Medium] This problem wasn't introduced by this patch, but
  hns3_alloc_ring_memory() leaks a newly created RX page pool when RX…

--

net: hns3: fix use-after-free in debugfs read during reset/unload

This patch adds a dbg_mutex to struct hnae3_handle. The enet-layer
debugfs show callbacks take it, and so do the reset uninit, client
uninit and ethtool ring resize paths while they free rings and vectors.
The debugfs readers also re-check INITED/RESETTING on every loop
iteration.

Closes: https://lore.kernel.org/all/a0853cd9-cab5-441d-b181-8ba97f2f58b0@huawei.com/

> The seq_file refactoring of hns3 debugfs removed the per-file entry
> state checks. Without them, debugfs show callbacks can access
> priv->ring, priv->tqp_vector, and ring->desc while these resources
> are being freed by reset, driver unload, or ethtool ring resize.

[ ... ]

> Fixes: eced3d1c41db ("net: hns3: use seq_file for files in queue/ in debugfs")

[Severity: Medium]
Is this Fixes tag complete?

Before eced3d1c41db, hns3_dbg_read() did a single INITED/RESETTING test
before running the dump. It shared no lock with
hns3_reset_notify_uninit_enet() or hns3_client_uninit. That is the same
window the changelog describes as "Plain state-bit checks leave a TOCTOU
window between check and use".

The ring resize race is older still. Since a723fb8efe29d3 ("net: hns3:
refine for set ring parameters"), hns3_set_ringparam() has freed the old
descriptors with:

	for (i = 0; i < h->kinfo.num_tqps * 2; i++)
		hns3_fini_ring(&tmp_rings[i]);

It never clears HNS3_NIC_STATE_INITED or sets HNS3_NIC_STATE_RESETTING.
So no state-bit check, old or new, would stop a concurrent tx_bd_info or
rx_bd_info reader from walking ring->desc. Also, eced3d1c41db only
converted rx_queue_info and queue_map.

Stable trees that predate eced3d1c41db have the debugfs vs ring resize
use-after-free. If backports go by this tag alone, would those trees be
skipped? Should a723fb8efe29d3 get a Fixes tag as well, and should the
changelog's wording about ethtool ring resize be adjusted?

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> index 1347edac76993..40d715924e588 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c

[ ... ]

> @@ -796,13 +823,9 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
>  	struct hns3_enet_ring *ring;
>  	u32 i;
>  
> -	if (!priv->ring) {
> -		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
> -		return -EFAULT;
> -	}
> -
> -	if (!priv->ring[h->kinfo.num_tqps].page_pool) {
> -		dev_err(&h->pdev->dev, "page pool is not initialized\n");
> +	mutex_lock(&priv->ae_handle->dbg_mutex);
> +	if (!priv->ring || !priv->ring[h->kinfo.num_tqps].page_pool) {
> +		mutex_unlock(&priv->ae_handle->dbg_mutex);
>  		return -EFAULT;
>  	}
>  
> @@ -810,14 +833,17 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
>  	seq_puts(s, "POOL_SIZE(PAGE_NUM)  ORDER  NUMA_ID  MAX_LEN\n");
>  
>  	for (i = 0; i < h->kinfo.num_tqps; i++) {
> -		if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
> -		    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
> -			return -EPERM;
> +		/* Per-iteration check bounds reset latency to one iteration. */
> +		if (hns3_dbg_is_device_busy(priv)) {
> +			mutex_unlock(&priv->ae_handle->dbg_mutex);
> +			return -EBUSY;
> +		}
>  
>  		ring = &priv->ring[(u32)(i + h->kinfo.num_tqps)];
>  		hns3_dump_page_pool_info(ring, s, i);

[Severity: High]
This is a pre-existing issue, but the check above only looks at the
page_pool of the first RX ring. The loop then passes every RX ring to
hns3_dump_page_pool_info(), which dereferences ring->page_pool without
a NULL check:

	READ_ONCE(ring->page_pool->pages_state_hold_cnt),
	atomic_read(&ring->page_pool->pages_state_release_cnt),
	ring->page_pool->p.pool_size,

hns3_alloc_page_pool() creates the pool separately for each RX ring. On
failure it leaves the pool NULL and ring init carries on:

	ring->page_pool = page_pool_create(&pp_params);
	if (IS_ERR(ring->page_pool)) {
		dev_warn(...);
		ring->page_pool = NULL;
	}

If the first RX ring got a pool and a later one did not (for example
after a transient allocation failure), can reading page_pool_info oops on
the NULL page_pool?

With this patch the oops would happen while dbg_mutex is held. Would
later calls to hns3_reset_notify_uninit_enet(), hns3_client_uninit() or
hns3_set_ringparam() then block forever in
mutex_lock(&handle->dbg_mutex)?

>  	}
>  
> +	mutex_unlock(&priv->ae_handle->dbg_mutex);
>  	return 0;
>  }
>  
> @@ -826,17 +852,23 @@ static int hns3_dbg_bd_info_show(struct seq_file *s, void *private)
>  	struct hns3_dbg_data *data = s->private;
>  	struct hnae3_handle *h = data->handle;
>  	struct hns3_nic_priv *priv = h->priv;
> +	int ret;
>  
> -	if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
> -	    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
> -		return -EBUSY;
> +	mutex_lock(&priv->ae_handle->dbg_mutex);
> +	if (!priv->ring) {
> +		mutex_unlock(&priv->ae_handle->dbg_mutex);
> +		return -EFAULT;
> +	}

[Severity: High]
Can a debugfs reader still dereference a freed priv->ring during reset
re-init?

hns3_reset_notify_uninit_enet(), hns3_client_uninit() and
hns3_set_ringparam() all take dbg_mutex, but
hns3_reset_notify_init_enet() never does. That function publishes a new
priv->ring while INITED is still clear. Its error unwind then frees
everything again without the mutex:

hns3_reset_notify_init_enet() {
	ret = hns3_get_ring_config(priv);
	...
err_init_irq_fail:
	hns3_uninit_all_ring(priv);
err_uninit_vector:
	hns3_nic_uninit_vector_data(priv);
err_dealloc_vector:
	hns3_nic_dealloc_vector_data(priv);
err_put_ring:
	hns3_put_ring_config(priv);
	...
}

The INITED/RESETTING entry check has been replaced with a bare priv->ring
check. So a reader in this window takes dbg_mutex unopposed and passes
the check. It then enters hns3_dbg_tx_bd_info() or hns3_dbg_rx_bd_info(),
and both dereference priv->ring before their first busy check:

	ring = &priv->ring[data->qid + data->handle->kinfo.num_tqps];
	for (i = 0; i < ring->desc_num; i++) {
		if (hns3_dbg_is_device_busy(priv))
			return -EBUSY;

If hns3_put_ring_config() runs at the same moment, the ring->desc_num
load reads devm-freed memory. If priv->ring has already been set back to
NULL, it faults on NULL plus an offset.

hns3_dbg_page_pool_info() looks to have the same problem, because its
check indexes priv->ring[h->kinfo.num_tqps] before any busy check.

Before this patch, hns3_dbg_bd_info_show() returned -EBUSY in this
window because INITED was clear. The changelog says the mutex "rejects a
reader that arrives after the writer has finished tearing down resources
(priv->ring is NULL by then)". That doesn't seem to hold once re-init has
reallocated priv->ring.

Would it work to take dbg_mutex in hns3_reset_notify_init_enet() around
the allocation, the error unwind and set_bit(HNS3_NIC_STATE_INITED)?
Alternatively, the busy check could stay ahead of the first priv->ring
dereference.

[ ... ]

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index 392653635bda2..cb6a23085be48 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> @@ -1258,6 +1258,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
>  	if (if_running)
>  		ndev->netdev_ops->ndo_stop(ndev);
>  
> +	mutex_lock(&h->dbg_mutex);
>  	hns3_change_all_ring_bd_num(priv, new_ringparam.tx_desc_num,
>  				    new_ringparam.rx_desc_num);
>  	hns3_change_rx_buf_len(ndev, new_ringparam.rx_buf_len);

[Severity: Medium]
This is a pre-existing issue, but can the hns3_init_all_ring() call that
follows here leak an RX page pool when buffer allocation fails?

hns3_alloc_ring_memory() creates the pool and then allocates the
buffers:

	if (!HNAE3_IS_TX_RING(ring)) {
		if (page_pool_enabled)
			hns3_alloc_page_pool(ring);

		ret = hns3_alloc_ring_buffers(ring);
		if (ret)
			goto out_with_desc;
	...
out_with_desc:
	hns3_free_desc(ring);
out_with_desc_cb:
	devm_kfree(ring_to_dev(ring), ring->desc_cb);

The error path never calls page_pool_destroy(ring->page_pool). And
hns3_init_all_ring() only unwinds the rings before the failing one:

	for (j = i - 1; j >= 0; j--)
		hns3_fini_ring(&priv->ring[j]);

After that the pointer is lost. hns3_set_ringparam() memcpy()s the saved
ring over priv->ring[i], and the probe/reset paths free priv->ring via
hns3_put_ring_config().

Would the page_pool, its ptr_ring and any pages that
hns3_alloc_ring_buffers() already returned to it be leaked for good?

> @@ -1276,6 +1277,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
>  		for (i = 0; i < h->kinfo.num_tqps * 2; i++)
>  			hns3_fini_ring(&tmp_rings[i]);
>  	}
> +	mutex_unlock(&h->dbg_mutex);
>  
>  	kfree(tmp_rings);
>  

[Severity: Medium]
This is a pre-existing issue, but suppose if_running is true and
hns3_init_all_ring() fails. The revert path restores the old ring
parameters, and then ret is overwritten just after this point:

	if (if_running)
		ret = ndev->netdev_ops->ndo_open(ndev);

	return ret;

If the reopen succeeds, does hns3_set_ringparam() return 0 for a resize
that was rolled back, so that ethnl_set_rings() reports success to
userspace?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021121426.3096838-1-shaojijie%40huawei.com