[PATCH v2] net: meth: Fix skb allocation failure handling in RX init

[PATCH v2] net: meth: Fix skb allocation failure handling in RX init

[Haoxiang Li]

meth_init_rx_ring() does not check the return value of alloc_skb().
If the allocation fails, the NULL skb is passed to skb_reserve() and
then dereferenced through skb->head.

Add check for alloc_skb() to prevent potential null pointer dereference.
And unwind the RX entries that were already allocated and DMA-mapped
before returning.

Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
---
Changes in v2:
 - Add error handling to free the resources that were already allocated. Thanks, Pavan.
 - Drop the fixes tag. Thanks, Andrew.
---
 drivers/net/ethernet/sgi/meth.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/sgi/meth.c b/drivers/net/ethernet/sgi/meth.c
index f7c3a5a766b7..bf5f47692023 100644
--- a/drivers/net/ethernet/sgi/meth.c
+++ b/drivers/net/ethernet/sgi/meth.c
@@ -228,6 +228,9 @@ static int meth_init_rx_ring(struct meth_private *priv)
 
 	for (i = 0; i < RX_RING_ENTRIES; i++) {
 		priv->rx_skbs[i] = alloc_skb(METH_RX_BUFF_SIZE, 0);
+		if (!priv->rx_skbs[i])
+			goto err_free_skbs;
+
 		/* 8byte status vector + 3quad padding + 2byte padding,
 		 * to put data on 64bit aligned boundary */
 		skb_reserve(priv->rx_skbs[i],METH_RX_HEAD);
@@ -240,6 +243,17 @@ static int meth_init_rx_ring(struct meth_private *priv)
 	}
         priv->rx_write = 0;
 	return 0;
+
+err_free_skbs:
+	while (i--) {
+		dma_unmap_single(&priv->pdev->dev, priv->rx_ring_dmas[i],
+				 METH_RX_BUFF_SIZE, DMA_FROM_DEVICE);
+		priv->rx_ring[i] = 0;
+		priv->rx_ring_dmas[i] = 0;
+		kfree_skb(priv->rx_skbs[i]);
+		priv->rx_skbs[i] = NULL;
+	}
+	return -ENOMEM;
 }
 static void meth_free_tx_ring(struct meth_private *priv)
 {
-- 
2.25.1

Re: [PATCH v2] net: meth: Fix skb allocation failure handling in RX init

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says:

Hi Haoxiang,

I think that the pre-existing issues flagged below do not need
to be considered in the context of this patch. But I am wondering
if you could take a look over the others.

---
net: meth: Fix skb allocation failure handling in RX init

meth_init_rx_ring() does not check the return value of alloc_skb().
If the allocation fails, the NULL skb is passed to skb_reserve() and
then dereferenced through skb->head.

> --- a/drivers/net/ethernet/sgi/meth.c
> +++ b/drivers/net/ethernet/sgi/meth.c
> @@ -228,6 +228,9 @@ static int meth_init_rx_ring(struct meth_private *priv)
>  
>  	for (i = 0; i < RX_RING_ENTRIES; i++) {
>  		priv->rx_skbs[i] = alloc_skb(METH_RX_BUFF_SIZE, 0);
> +		if (!priv->rx_skbs[i])
> +			goto err_free_skbs;
> +
>  		/* 8byte status vector + 3quad padding + 2byte padding,
>  		 * to put data on 64bit aligned boundary */
>  		skb_reserve(priv->rx_skbs[i],METH_RX_HEAD);

[ ... ]

> @@ -240,6 +243,17 @@ static int meth_init_rx_ring(struct meth_private *priv)
>  	}
>          priv->rx_write = 0;
>  	return 0;
> +
> +err_free_skbs:
> +	while (i--) {
> +		dma_unmap_single(&priv->pdev->dev, priv->rx_ring_dmas[i],
> +				 METH_RX_BUFF_SIZE, DMA_FROM_DEVICE);

[Severity: Critical]
During the allocation loop, each successfully mapped DMA address is pushed
immediately to the hardware via the mace->eth.rx_fifo register. Unmapping
them here frees the memory, but the hardware FIFO still retains the physical
addresses.

When meth_init_rx_ring() returns -ENOMEM to meth_tx_timeout(), the error is
ignored and the RX DMA engine is enabled anyway:

meth_tx_timeout()
    ...
    meth_init_rx_ring(priv);
    priv->dma_ctrl |= METH_DMA_TX_EN | METH_DMA_RX_EN | METH_DMA_RX_INT_EN;

Will this cause the hardware to DMA incoming packet data into the unmapped
and potentially reallocated memory?

> +		priv->rx_ring[i] = 0;

[Severity: High]
If the hardware remains active as described above, will the RX interrupt
handler dereference this NULL pointer?

When the hardware receives a packet, it raises an RX interrupt and meth_rx()
attempts to process it:

meth_rx()
    ...
    status = priv->rx_ring[priv->rx_write]->status.raw;

Since the pointer at this index was cleared in the error path, this looks
like it will cause a kernel panic.

> +		priv->rx_ring_dmas[i] = 0;
> +		kfree_skb(priv->rx_skbs[i]);
> +		priv->rx_skbs[i] = NULL;

[Severity: Critical]
Does this partial cleanup lead to a double free of socket buffers?

When meth_init_rx_ring() fails at index N, this loop zeroes out elements 0
through N-1. The elements from N through 63 retain their dangling pointers
from the previous execution of meth_free_rx_ring(), which fails to clear SKB
pointers after freeing them.

Since meth_tx_timeout() ignores the error and leaves the device running, the
next time meth_free_rx_ring() is called, it iterates over all 64 elements
again. Will this call kfree_skb() on the dangling pointers in indices N
through 63?

> +	}
> +	return -ENOMEM;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but dma_map_single() is called in
meth_init_rx_ring() without checking for mapping errors:

meth_init_rx_ring()
    ...
    priv->rx_ring_dmas[i] = dma_map_single(&priv->pdev->dev,
                                           priv->rx_ring[i],
                                           METH_RX_BUFF_SIZE,
                                           DMA_FROM_DEVICE);
    mace->eth.rx_fifo = priv->rx_ring_dmas[i];

If the mapping fails, an error cookie is passed directly to the hardware.
Could this lead to IOMMU faults or memory corruption when DMA bounce buffers
are exhausted?

[Severity: High]
This is a pre-existing issue, but there appears to be a race condition due
to lockless processing in meth_rx().

The interrupt handler drops the spinlock before processing the ring:

meth_rx()
    ...
    spin_unlock_irqrestore(&priv->meth_lock, flags);
    ...
    while (priv->rx_write != fifo_rptr) {

Concurrently, meth_tx_timeout() can acquire the lock and destroy the RX rings,
falsely assuming it is protected:

meth_tx_timeout()
    ...
    spin_lock_irqsave(&priv->meth_lock,flags);
    meth_reset(dev);
    ...
    meth_free_rx_ring(priv);

If meth_free_rx_ring() frees the SKBs while meth_rx() is still executing the
lockless while loop, could this result in a use-after-free or NULL pointer
dereference?