diff mbox series

[net] net: enetc: zeroize buffer descriptors in enetc_dma_alloc_bdr()

Message ID 20221024172049.4187400-1-vladimir.oltean@nxp.com
State New
Headers
Series [net] net: enetc: zeroize buffer descriptors in enetc_dma_alloc_bdr() |

Commit Message

Vladimir Oltean Oct. 24, 2022, 5:20 p.m. UTC 
  Under memory pressure, enetc_refill_rx_ring() may fail, and when called
during the enetc_open() -> enetc_setup_rxbdr() procedure, this is not
checked for.

An extreme case of memory pressure will result in exactly zero buffers
being allocated for the RX ring, and in such a case it is expected that
hardware drops all RX packets due to lack of buffers.

The hardware drop happens, however the driver has undefined behavior and
may even crash. Explanation follows below.

The enetc NAPI poll procedure is shared between RX and TX conf, and
enetc_poll() calls enetc_clean_rx_ring() even if the reason why NAPI was
scheduled is TX.

The enetc_clean_rx_ring() function (and its XDP derivative) is not
prepared to handle such a condition. It has this loop exit condition:

		rxbd = enetc_rxbd(rx_ring, i);
		bd_status = le32_to_cpu(rxbd->r.lstatus);
		if (!bd_status)
			break;

otherwise said, the NAPI poll procedure does not look at the Producer
Index of the RX ring, instead it just walks circularly through the
descriptors until it finds one which is not Ready.

The problem is that the enetc_rxbd(rx_ring, i) RX descriptor is only
initialized by enetc_refill_rx_ring() if page allocation has succeeded.

So if memory allocation ever failed, enetc_clean_rx_ring() looks at
rxbd->r.lstatus as an exit condition, but "rxbd" itself is uninitialized
memory. If it contains junk, then junk buffers will be processed.

To fix the problem, memset the DMA coherent area used for BD rings.
This makes all BDs be "not ready" by default, which makes
enetc_clean_rx_ring() exit early from the BD processing loop when there
is no valid buffer available.

Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Claudiu Manoil Oct. 25, 2022, 7:41 a.m. UTC | #1 
> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Sent: Monday, October 24, 2022 8:21 PM
[...]
> Subject: [PATCH net] net: enetc: zeroize buffer descriptors in
> enetc_dma_alloc_bdr()
> 

Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>
Vladimir Oltean Oct. 25, 2022, 11:31 p.m. UTC | #2 
On Mon, Oct 24, 2022 at 08:20:49PM +0300, Vladimir Oltean wrote:
> Under memory pressure, enetc_refill_rx_ring() may fail, and when called
> during the enetc_open() -> enetc_setup_rxbdr() procedure, this is not
> checked for.

Please don't apply this yet, I'm still investigating a crash which has
the symptoms described here, and I'm not sure why the patch didn't fix it.
Vladimir Oltean Oct. 26, 2022, 12:14 p.m. UTC | #3 
On Wed, Oct 26, 2022 at 02:31:52AM +0300, Vladimir Oltean wrote:
> On Mon, Oct 24, 2022 at 08:20:49PM +0300, Vladimir Oltean wrote:
> > Under memory pressure, enetc_refill_rx_ring() may fail, and when called
> > during the enetc_open() -> enetc_setup_rxbdr() procedure, this is not
> > checked for.
> 
> Please don't apply this yet, I'm still investigating a crash which has
> the symptoms described here, and I'm not sure why the patch didn't fix it.

Superseded by v2:
https://patchwork.kernel.org/project/netdevbpf/patch/20221026121330.2042989-1-vladimir.oltean@nxp.com/
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 54bc92fc6bf0..1c272ca3a05a 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1738,18 +1738,21 @@  void enetc_get_si_caps(struct enetc_si *si)
 
 static int enetc_dma_alloc_bdr(struct enetc_bdr *r, size_t bd_size)
 {
-	r->bd_base = dma_alloc_coherent(r->dev, r->bd_count * bd_size,
-					&r->bd_dma_base, GFP_KERNEL);
+	size_t size = r->bd_count * bd_size;
+
+	r->bd_base = dma_alloc_coherent(r->dev, size, &r->bd_dma_base,
+					GFP_KERNEL);
 	if (!r->bd_base)
 		return -ENOMEM;
 
 	/* h/w requires 128B alignment */
 	if (!IS_ALIGNED(r->bd_dma_base, 128)) {
-		dma_free_coherent(r->dev, r->bd_count * bd_size, r->bd_base,
-				  r->bd_dma_base);
+		dma_free_coherent(r->dev, size, r->bd_base, r->bd_dma_base);
 		return -EINVAL;
 	}
 
+	memset(r->bd_base, 0, size);
+
 	return 0;
 }