[PATCH] rtl818x_pci: Fix potential memory leaks in rtl8180_init_rx_ring()

[PATCH] rtl818x_pci: Fix potential memory leaks in rtl8180_init_rx_ring()

[Abdun Nihaal]

In rtl8180_init_rx_ring(), memory is allocated for skb packets and DMA
allocations in a loop. When an allocation fails, the previously
successful allocations are not freed on exit.

Fixes: f653211197f3 ("Add rtl8180 wireless driver")
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
Compile tested only.

 .../wireless/realtek/rtl818x/rtl8180/dev.c    | 24 ++++++++++++-------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
index 2905baea6239..898611ccb400 100644
--- a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
@@ -1023,11 +1023,8 @@ static int rtl8180_init_rx_ring(struct ieee80211_hw *dev)
 		dma_addr_t *mapping;
 		entry = priv->rx_ring + priv->rx_ring_sz*i;
 		if (!skb) {
-			dma_free_coherent(&priv->pdev->dev,
-					  priv->rx_ring_sz * 32,
-					  priv->rx_ring, priv->rx_ring_dma);
 			wiphy_err(dev->wiphy, "Cannot allocate RX skb\n");
-			return -ENOMEM;
+			goto error;
 		}
 		priv->rx_buf[i] = skb;
 		mapping = (dma_addr_t *)skb->cb;
@@ -1037,11 +1034,9 @@ static int rtl8180_init_rx_ring(struct ieee80211_hw *dev)
 
 		if (dma_mapping_error(&priv->pdev->dev, *mapping)) {
 			kfree_skb(skb);
-			dma_free_coherent(&priv->pdev->dev,
-					  priv->rx_ring_sz * 32,
-					  priv->rx_ring, priv->rx_ring_dma);
+			priv->rx_buf[i] = NULL;
 			wiphy_err(dev->wiphy, "Cannot map DMA for RX skb\n");
-			return -ENOMEM;
+			goto error;
 		}
 
 		entry->rx_buf = cpu_to_le32(*mapping);
@@ -1050,6 +1045,19 @@ static int rtl8180_init_rx_ring(struct ieee80211_hw *dev)
 	}
 	entry->flags |= cpu_to_le32(RTL818X_RX_DESC_FLAG_EOR);
 	return 0;
+error:
+	while (i--) {
+		struct sk_buff *skb = priv->rx_buf[i];
+		priv->rx_buf[i] = NULL;
+		dma_unmap_single(&priv->pdev->dev, *((dma_addr_t *)skb->cb),
+				 MAX_RX_SIZE, DMA_FROM_DEVICE);
+		kfree_skb(skb);
+	}
+	dma_free_coherent(&priv->pdev->dev,
+			  priv->rx_ring_sz * 32,
+			  priv->rx_ring, priv->rx_ring_dma);
+	priv->rx_ring = NULL;
+	return -ENOMEM;
 }
 
 static void rtl8180_free_rx_ring(struct ieee80211_hw *dev)
-- 
2.43.0

RE: [PATCH] rtl818x_pci: Fix potential memory leaks in rtl8180_init_rx_ring()

[Ping-Ke Shih]

Abdun Nihaal <nihaal@cse.iitm.ac.in> wrote:
> In rtl8180_init_rx_ring(), memory is allocated for skb packets and DMA
> allocations in a loop. When an allocation fails, the previously
> successful allocations are not freed on exit.
> 
> Fixes: f653211197f3 ("Add rtl8180 wireless driver")
> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
> ---
> Compile tested only.
> 

I'm surprised that people work on this old driver, and how did you find
this flaw?


It seems like rtl8180_free_rx_ring() does all things you are adding, so
just goto err_free_rings?

@@ -1130,7 +1131,7 @@ static int rtl8180_start(struct ieee80211_hw *dev)

        ret = rtl8180_init_rx_ring(dev);
        if (ret)
-               return ret;
+               goto err_free_rings;

        for (i = 0; i < (dev->queues + 1); i++)
                if ((ret = rtl8180_init_tx_ring(dev, i, 16)))

Re: [PATCH] rtl818x_pci: Fix potential memory leaks in rtl8180_init_rx_ring()

[Abdun Nihaal]

On Fri, Nov 14, 2025 at 07:20:05AM +0000, Ping-Ke Shih wrote:
> 
> I'm surprised that people work on this old driver, and how did you find
> this flaw?

I'm building a static analysis tool for my research. This issue showed
up when I ran the tool on all kernel drivers.

> It seems like rtl8180_free_rx_ring() does all things you are adding, so
> just goto err_free_rings?
> 
> @@ -1130,7 +1131,7 @@ static int rtl8180_start(struct ieee80211_hw *dev)
> 
>         ret = rtl8180_init_rx_ring(dev);
>         if (ret)
> -               return ret;
> +               goto err_free_rings;
> 
>         for (i = 0; i < (dev->queues + 1); i++)
>                 if ((ret = rtl8180_init_tx_ring(dev, i, 16)))

Yes, calling rtl8180_free_rx_ring() is much simpler. But the error path
of rtl8180_init_tx_ring() is not setting the freed object to NULL, which
could lead to a double free. Moreover, I feel handling it inside the 
rtl8180_init_tx_ring() function itself is better, to have it allocate
all or none.

I'll simplify the error path of rtl8180_init_tx_ring() by calling
rtl8180_free_rx_ring(), and send a v2 patch.

Regards,
Nihaal