netdev - RE: [PATCH] gianfar: fix jumbo packets+napi+rx overrun crash

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB8PR04MB67643BBB399B02ACBA48C06F96979@DB8PR04MB6764.eurprd04.prod.outlook.com>
Date:   Thu, 4 Mar 2021 10:05:59 +0000
From:   Claudiu Manoil <claudiu.manoil@....com>
To:     "michael-dev@...i-braun.de" <michael-dev@...i-braun.de>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] gianfar: fix jumbo packets+napi+rx overrun crash

>-----Original Message-----
>From: michael-dev@...i-braun.de <michael-dev@...i-braun.de>
>Sent: Thursday, March 4, 2021 5:13 AM
>To: Claudiu Manoil <claudiu.manoil@....com>
>Cc: netdev@...r.kernel.org; Michael Braun <michael-dev@...i-braun.de>
>Subject: [PATCH] gianfar: fix jumbo packets+napi+rx overrun crash
>
>From: Michael Braun <michael-dev@...i-braun.de>
>
>When using jumbo packets and overrunning rx queue with napi enabled,
>the following sequence is observed in gfar_add_rx_frag:
>

Hi,

Could you help provide some context, e.g.:
On what board/soc were you able to trigger this issue?
How often does the overrun occur? What's the use case? Is the issue triggered with smaller packets than 9600B?
Increasing the Rx ring size does significantly reduce ring overruns?
Thanks.

>   | lstatus                              |       | skb                   |
>t  | lstatus,  size, flags                | first | len, data_len, *ptr   |
>---+--------------------------------------+-------+-----------------------+
>13 | 18002348, 9032, INTERRUPT LAST       | 0     | 9600, 8000,  f554c12e |
>12 | 10000640, 1600, INTERRUPT            | 0     | 8000, 6400,  f554c12e |
>11 | 10000640, 1600, INTERRUPT            | 0     | 6400, 4800,  f554c12e |
>10 | 10000640, 1600, INTERRUPT            | 0     | 4800, 3200,  f554c12e |
>09 | 10000640, 1600, INTERRUPT            | 0     | 3200, 1600,  f554c12e |
>08 | 14000640, 1600, INTERRUPT FIRST      | 0     | 1600, 0,     f554c12e |
>07 | 14000640, 1600, INTERRUPT FIRST      | 1     | 0,    0,     f554c12e |
>06 | 1c000080, 128,  INTERRUPT LAST FIRST | 1     | 0,    0,     abf3bd6e |
>05 | 18002348, 9032, INTERRUPT LAST       | 0     | 8000, 6400,  c5a57780 |
>04 | 10000640, 1600, INTERRUPT            | 0     | 6400, 4800,  c5a57780 |
>03 | 10000640, 1600, INTERRUPT            | 0     | 4800, 3200,  c5a57780 |
>02 | 10000640, 1600, INTERRUPT            | 0     | 3200, 1600,  c5a57780 |
>01 | 10000640, 1600, INTERRUPT            | 0     | 1600, 0,     c5a57780 |
>00 | 14000640, 1600, INTERRUPT FIRST      | 1     | 0,    0,     c5a57780 |
>
>So at t=7 a new packets is started but not finished, probably due to rx
>overrun - but rx overrun is not indicated in the flags. Instead a new
>packets starts at t=8. This results in skb->len to exceed size for the LAST
>fragment at t=13 and thus a negative fragment size added to the skb.
>
>This then crashes:
>
>kernel BUG at include/linux/skbuff.h:2277!
>Oops: Exception in kernel mode, sig: 5 [#1]
>...
>NIP [c04689f4] skb_pull+0x2c/0x48
>LR [c03f62ac] gfar_clean_rx_ring+0x2e4/0x844
>Call Trace:
>[ec4bfd38] [c06a84c4] _raw_spin_unlock_irqrestore+0x60/0x7c (unreliable)
>[ec4bfda8] [c03f6a44] gfar_poll_rx_sq+0x48/0xe4
>[ec4bfdc8] [c048d504] __napi_poll+0x54/0x26c
>[ec4bfdf8] [c048d908] net_rx_action+0x138/0x2c0
>[ec4bfe68] [c06a8f34] __do_softirq+0x3a4/0x4fc
>[ec4bfed8] [c0040150] run_ksoftirqd+0x58/0x70
>[ec4bfee8] [c0066ecc] smpboot_thread_fn+0x184/0x1cc
>[ec4bff08] [c0062718] kthread+0x140/0x144
>[ec4bff38] [c0012350] ret_from_kernel_thread+0x14/0x1c
>
>This patch fixes this by checking for computed LAST fragment size, so a
>negative sized fragment is never added.
>In order to prevent the newer rx frame from getting corrupted, the FIRST
>flag is checked to discard the incomplete older frame.
>
>Signed-off-by: Michael Braun <michael-dev@...i-braun.de>
>---
> drivers/net/ethernet/freescale/gianfar.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/drivers/net/ethernet/freescale/gianfar.c
>b/drivers/net/ethernet/freescale/gianfar.c
>index 541de32ea662..2aecae23bfd0 100644
>--- a/drivers/net/ethernet/freescale/gianfar.c
>+++ b/drivers/net/ethernet/freescale/gianfar.c
>@@ -2390,6 +2390,10 @@ static bool gfar_add_rx_frag(struct gfar_rx_buff
>*rxb, u32 lstatus,
> 		if (lstatus & BD_LFLAG(RXBD_LAST))
> 			size -= skb->len;
>
>+		WARN(size < 0, "gianfar: rx fragment size underflow");
>+		if (size < 0)
>+			return false;
>+
> 		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
> 				rxb->page_offset + RXBUF_ALIGNMENT,
> 				size, GFAR_RXB_TRUESIZE);
>@@ -2552,6 +2556,16 @@ static int gfar_clean_rx_ring(struct gfar_priv_rx_q
>*rx_queue,
> 		if (lstatus & BD_LFLAG(RXBD_EMPTY))
> 			break;
>
>+		/* lost RXBD_LAST descriptor due to overrun */
>+		if (skb &&
>+		    (lstatus & BD_LFLAG(RXBD_FIRST))) {
>+			/* discard faulty buffer */
>+			dev_kfree_skb(skb);
>+			skb = NULL;
>+
>+			/* can continue normally */
>+		}
>+

This is indeed an invalid state. If you hit this condition, discarding the skb is the right thing to do.

Acked-by: Claudiu Manoil <claudiu.manoil@....com>

> 		/* order rx buffer descriptor reads */
> 		rmb();
>
>--
>2.20.1