Re: [PATCH] net: ethernet: mtk_wed: fix double free in mtk_wed_rx_reset

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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] The patch fixes the immediate double free by removing the second
  mtk_wed_hwrro_free_buffer() call from mtk_wed_rx_reset().

--

net: ethernet: mtk_wed: fix double free in mtk_wed_rx_reset

mtk_wed_free_rx_buffer() already calls mtk_wed_hwrro_free_buffer(). The
patch therefore drops the second mtk_wed_hwrro_free_buffer() call from
mtk_wed_rx_reset(), which caused a double free of the hw_rro buffers on
WED v3.

Link: https://sashiko.dev/#/map/216156

> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> index f40ac5794341c..5e9674c9e63ab 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> @@ -1723,7 +1723,6 @@ mtk_wed_rx_reset(struct mtk_wed_device *dev)
>  				   false);
>  	}
>  	mtk_wed_free_rx_buffer(dev);
> -	mtk_wed_hwrro_free_buffer(dev);

[Severity: High]
This removes the duplicate call, but the free helpers still never clear the
pointers they free. Can the stale rx_buf_ring.desc, hw_rro.desc and
hw_rro.pages still get freed a second time later?

mtk_wed_free_rx_buffer() frees the descriptor ring and leaves
dev->rx_buf_ring.desc set:

drivers/net/ethernet/mediatek/mtk_wed.c:mtk_wed_free_rx_buffer() {
	...
	dev->wlan.release_rx_buf(dev);
	dma_free_coherent(dev->hw->dev, dev->rx_buf_ring.size * sizeof(*desc),
			  desc, dev->rx_buf_ring.desc_phys);

	mtk_wed_hwrro_free_buffer(dev);
}

mtk_wed_hwrro_free_buffer() works the same way. It unmaps and frees the
pages, calls dma_free_coherent() on hw_rro.desc and kfree()s hw_rro.pages,
but leaves both pointers set.

After the reset path:

mtk_wed_device_dma_reset()
  mtk_wed_reset_dma()
    mtk_wed_rx_reset()
      mtk_wed_free_rx_buffer()

these pointers stay dangling until mtk_wed_start()->mtk_wed_rx_buffer_alloc()
overwrites them. Some paths seem to skip that step.

In mt7915_dma_start(), mtk_wed_device_start() is never called if the WO
rx stats message fails:

	ret = mt7915_mcu_wed_enable_rx_stats(dev);
	if (ret)
		return ret;

	mtk_wed_device_start(&dev->mt76.mmio.wed, wed_irq_mask);

mtk_wed_rx_buffer_alloc() returns -ENOMEM before it assigns
dev->rx_buf_ring.desc if dma_alloc_coherent() fails.

mtk_wed_hwrro_buffer_alloc() returns -ENOMEM in two places. If the
page_list allocation fails, it returns before assigning hw_rro.pages. If
the desc dma_alloc_coherent() fails, it returns before assigning
hw_rro.desc.

In any of these cases, the next unbind or remove would do:

__mtk_wed_detach()
  mtk_wed_free_rx_rings()
    mtk_wed_free_rx_buffer()
      dma_free_coherent() on the old rx_buf_ring.desc
      mtk_wed_hwrro_free_buffer()
        reads page_list[] from the already kfree()d hw_rro.pages
        dma_unmap_page() and __free_page() on already freed pages
        dma_free_coherent() on the old hw_rro.desc
        kfree() on the old hw_rro.pages

The if (!desc) and if (!page_list) checks don't help here, because the
pointers are still non-NULL.

Would it be better to set rx_buf_ring.desc, hw_rro.desc and hw_rro.pages
to NULL in mtk_wed_free_rx_buffer() and mtk_wed_hwrro_free_buffer()? The
commit message names the same cause:

  "This would cause double free because dma_free_coherent doesn't set
  freed desc to NULL."

Clearing the pointers would also have made the removed duplicate call
harmless.

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104-double-free-v1-1-efa52326702f%40gmail.com