netdev - RE: [PATCH] gianfar: fix size of fragmented frames

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: <AM5PR0401MB25613B657270085A3EE4324F96160@AM5PR0401MB2561.eurprd04.prod.outlook.com>
Date:   Fri, 19 Aug 2016 16:46:46 +0000
From:   Claudiu Manoil <claudiu.manoil@....com>
To:     Zefir Kurtisi <zefir.kurtisi@...atec.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "claudiu.manoil@...escale.com" <claudiu.manoil@...escale.com>
Subject: RE: [PATCH] gianfar: fix size of fragmented frames

>-----Original Message-----
>From: Zefir Kurtisi [mailto:zefir.kurtisi@...atec.com]
>Sent: Friday, August 19, 2016 12:16 PM
>To: netdev@...r.kernel.org
>Cc: claudiu.manoil@...escale.com
>Subject: [PATCH] gianfar: fix size of fragmented frames
>
>The eTSEC RxBD 'Data Length' field is context depening:
>for the last fragment it contains the full frame size,
>while fragments contain the fragment size, which equals
>the value written to register MRBLR.
>

According to RM the last fragment has the whole packet length indeed,
and this should apply to fragmented frames too:

" Data length, written by the eTSEC.
Data length is the number of octets written by the eTSEC into this BD's data buffer if L is cleared
(the value is equal to MRBLR), or, if L is set, the length of the frame including CRC, FCB (if
RCTRL[PRSDEP > 00), preamble (if MACCFG2[PreAmRxEn]=1), time stamp (if RCTRL[TS] = 1)
and any padding (RCTRL[PAL]). "

>This differentiation is missing in the gianfar driver,
>which causes data corruption as soon as the hardware
>starts to fragment receiving frames. As a result, the
>size of fragmented frames is increased by
>(nr_frags - 1) * MRBLR
>
>We first noticed this issue working with DSA, where a
>1540 octet frame is fragmented by the hardware and
>reconstructed by the driver as 3076 octet frame.
>
>This patch fixes the problem by adjusting the size of
>the last fragment.
>
>Signed-off-by: Zefir Kurtisi <zefir.kurtisi@...atec.com>
>---
> drivers/net/ethernet/freescale/gianfar.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/net/ethernet/freescale/gianfar.c
>b/drivers/net/ethernet/freescale/gianfar.c
>index d20935d..4187280 100644
>--- a/drivers/net/ethernet/freescale/gianfar.c
>+++ b/drivers/net/ethernet/freescale/gianfar.c
>@@ -2922,17 +2922,24 @@ static bool gfar_add_rx_frag(struct gfar_rx_buff *rxb,
>u32 lstatus,
> {
> 	unsigned int size = lstatus & BD_LENGTH_MASK;
> 	struct page *page = rxb->page;
>+	bool last = !!(lstatus & BD_LFLAG(RXBD_LAST));
>
> 	/* Remove the FCS from the packet length */
>-	if (likely(lstatus & BD_LFLAG(RXBD_LAST)))
>+	if (last)
> 		size -= ETH_FCS_LEN;
>
>-	if (likely(first))
>+	if (likely(first)) {
> 		skb_put(skb, size);
>-	else
>-		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
>-				rxb->page_offset + RXBUF_ALIGNMENT,
>-				size, GFAR_RXB_TRUESIZE);
>+	} else {
>+		/* the last fragments' length contains the full frame length */
>+		if (last)
>+			size -= skb->len;

While I agree with your finding, I don't think this works for packets having more
than 2 buffers (head + 1 fragment).
How's this supposed to work for a skb with 2 fragments, for instance?
I think this needs more thoughtful consideration.

>+
>+		if (size > 0 && size <= GFAR_RXB_SIZE)

Why do you need this check?
The h/w ensures that the buffers won't exceed GFAR_RXB_SIZE
(which is MRBL), size 0 is also not possible.

>+			skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
>+					rxb->page_offset + RXBUF_ALIGNMENT,
>+					size, GFAR_RXB_TRUESIZE);
>+	}
>
> 	/* try reuse page */
> 	if (unlikely(page_count(page) != 1))
>--
>2.7.4