diff mbox series

net: stmmac: fix memory corruption with large MTUs

Message ID 20190318213608.612-1-aaro.koskinen@iki.fi
State Accepted
Delegated to: David Miller
Headers show
Series net: stmmac: fix memory corruption with large MTUs | expand

Commit Message

Aaro Koskinen March 18, 2019, 9:36 p.m. UTC 
From: Aaro Koskinen <aaro.koskinen@nokia.com>

When using 16K DMA buffers and ring mode, the DES3 refill is not working
correctly as the function is using a bogus pointer for checking the
private data. As a result stale pointers will remain in the RX descriptor
ring, so DMA will now likely overwrite/corrupt some already freed memory.

As simple reproducer, just receive some UDP traffic:

	# ifconfig eth0 down; ifconfig eth0 mtu 9000; ifconfig eth0 up
	# iperf3 -c 192.168.253.40 -u -b 0 -R

If you didn't crash by now check the RX descriptors to find non-contiguous
RX buffers:

	cat /sys/kernel/debug/stmmaceth/eth0/descriptors_status
	[...]
	1 [0x2be5020]: 0xa3220321 0x9ffc1ffc 0x72d70082 0x130e207e
					     ^^^^^^^^^^^^^^^^^^^^^
	2 [0x2be5040]: 0xa3220321 0x9ffc1ffc 0x72998082 0x1311a07e
					     ^^^^^^^^^^^^^^^^^^^^^

A simple ping test will now report bad data:

	# ping -s 8200 192.168.253.40
	PING 192.168.253.40 (192.168.253.40) 8200(8228) bytes of data.
	8208 bytes from 192.168.253.40: icmp_seq=1 ttl=64 time=1.00 ms
	wrong data byte #8144 should be 0xd0 but was 0x88

Fix the wrong pointer. Also we must refill DES3 only if the DMA buffer
size is 16K.

Fixes: 54139cf3bb33 ("net: stmmac: adding multiple buffers for rx")
Signed-off-by: Aaro Koskinen <aaro.koskinen@nokia.com>
---
 drivers/net/ethernet/stmicro/stmmac/ring_mode.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Jose Abreu March 19, 2019, 10:36 a.m. UTC | #1 
On 3/18/2019 9:36 PM, Aaro Koskinen wrote:
>  	/* Fill DES3 in case of RING mode */
> -	if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
> +	if (priv->dma_buf_sz == BUF_SIZE_16KiB)

Shouldn't this be: "if (priv->dma_buf_sz > BUF_SIZE_8KiB)" ?

Thanks,
Jose Miguel Abreu
Koskinen, Aaro (Nokia - FI/Espoo) March 19, 2019, 11:07 a.m. UTC | #2 
Hi,

From: Jose Abreu [jose.abreu@synopsys.com]:
> On 3/18/2019 9:36 PM, Aaro Koskinen wrote:
> >       /* Fill DES3 in case of RING mode */
> > -     if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
> > +     if (priv->dma_buf_sz == BUF_SIZE_16KiB)
>
> Shouldn't this be: "if (priv->dma_buf_sz > BUF_SIZE_8KiB)" ?

I think it should be the same as in stmmac_init_rx_buffers():

        if (priv->dma_buf_sz == BUF_SIZE_16KiB)
                stmmac_init_desc3(priv, p);

A.
Jose Abreu March 19, 2019, 2:42 p.m. UTC | #3 
On 3/19/2019 11:07 AM, Koskinen, Aaro (Nokia - FI/Espoo) wrote:
> Hi,
> 
> From: Jose Abreu [jose.abreu@synopsys.com]:
>> On 3/18/2019 9:36 PM, Aaro Koskinen wrote:
>>>       /* Fill DES3 in case of RING mode */
>>> -     if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
>>> +     if (priv->dma_buf_sz == BUF_SIZE_16KiB)
>>
>> Shouldn't this be: "if (priv->dma_buf_sz > BUF_SIZE_8KiB)" ?
> 
> I think it should be the same as in stmmac_init_rx_buffers():
> 
>         if (priv->dma_buf_sz == BUF_SIZE_16KiB)
>                 stmmac_init_desc3(priv, p);
> 
> A.
> 

Hmm, yeah makes sense. I was under impression that XGMAC had a >
16KB buffer but I was wrong.

This change looks okay then.

Acked-by: Jose Abreu <joabreu@synopsys.com>

Thanks,
Jose Miguel Abreu
David Miller March 19, 2019, 8:33 p.m. UTC | #4 
From: Aaro Koskinen <aaro.koskinen@iki.fi>
Date: Mon, 18 Mar 2019 23:36:08 +0200

> From: Aaro Koskinen <aaro.koskinen@nokia.com>
> 
> When using 16K DMA buffers and ring mode, the DES3 refill is not working
> correctly as the function is using a bogus pointer for checking the
> private data. As a result stale pointers will remain in the RX descriptor
> ring, so DMA will now likely overwrite/corrupt some already freed memory.
> 
> As simple reproducer, just receive some UDP traffic:
 ...
> Fixes: 54139cf3bb33 ("net: stmmac: adding multiple buffers for rx")
> Signed-off-by: Aaro Koskinen <aaro.koskinen@nokia.com>

Applied and queued up for -stable, thank you.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
index f936166d8910..4d9bcb4d0378 100644
--- a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
+++ b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
@@ -113,10 +113,11 @@  static unsigned int is_jumbo_frm(int len, int enh_desc)
 
 static void refill_desc3(void *priv_ptr, struct dma_desc *p)
 {
-	struct stmmac_priv *priv = (struct stmmac_priv *)priv_ptr;
+	struct stmmac_rx_queue *rx_q = priv_ptr;
+	struct stmmac_priv *priv = rx_q->priv_data;
 
 	/* Fill DES3 in case of RING mode */
-	if (priv->dma_buf_sz >= BUF_SIZE_8KiB)
+	if (priv->dma_buf_sz == BUF_SIZE_16KiB)
 		p->des3 = cpu_to_le32(le32_to_cpu(p->des2) + BUF_SIZE_8KiB);
 }