netdev - [PATCH v3] net: Do not include padding in TCP GRO checksum

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-next>] [day] [month] [year] [list]
Message-ID: <20131115225856.6988.69733.stgit@ahduyck-fpga.jf.intel.com>
Date:	Fri, 15 Nov 2013 15:00:34 -0800
From:	Alexander Duyck <alexander.h.duyck@...el.com>
To:	davem@...emloft.net
Cc:	netdev@...r.kernel.org, edumazet@...gle.com,
	herbert@...dor.apana.org
Subject: [PATCH v3] net: Do not include padding in TCP GRO checksum

In some recent tests I found the TCP checksum was being treated as valid
for certain frames with padding on them.  On closer inspection I found the
issue was that GRO was using the skb->len instead of the length recorded in
the IP/IPv6 header to determine the number of bytes to checksum.  As such
padded frames that actually had invalid checksums generated by adding the
padding to the checksum were being incorrectly tagged as valid.

This change corrects that by using the tot_len from IPv4 headers and the
payload_len from IPv6 headers to compute the total number of bytes to be
included in the checksum.

To address the fact that skb->csum is invalid when a padded frame is
received I have updated the code to fall though to the CHECKSUM_NONE path
for CHECKSUM_COMPLETE frames that contain padding.

Signed-off-by: Alexander Duyck <alexander.h.duyck@...el.com>
---

v2: Update byte ordering of tot_len and payload_len so it is in host order.
    Updated CHECKSUM_COMPLETE path so it falls back through CHECKSUM_NONE for
    padded frames since this is how it is handled in ip_rcv.

    I have tested and verified the CHECKSUM_NONE path works, but I don't have
    any adapters that generate CHECKSUM_COMPLETE to test with.
v3: Added check to handle case where length is greater than skb_gro_len.

 net/ipv4/tcp_offload.c   |   30 +++++++++++++++++++++---------
 net/ipv6/tcpv6_offload.c |   31 +++++++++++++++++++++----------
 2 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index a2b68a1..b32f6c3 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -273,26 +273,38 @@ static int tcp_v4_gso_send_check(struct sk_buff *skb)
 static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *skb)
 {
 	const struct iphdr *iph = skb_gro_network_header(skb);
+	int length = ntohs(iph->tot_len);
 	__wsum wsum;
 	__sum16 sum;
 
+	/* adjust for any offsets */
+	length += skb_network_offset(skb) - skb_gro_offset(skb);
+
+	/* verify the entire packet is here */
+	if (length > skb_gro_len(skb))
+		goto flush;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
-		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
-				  skb->csum)) {
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
-			break;
-		}
+		if (length == skb_gro_len(skb)) {
+			if (!tcp_v4_check(length, iph->saddr, iph->daddr,
+					   skb->csum)) {
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
+				break;
+			}
 flush:
-		NAPI_GRO_CB(skb)->flush = 1;
-		return NULL;
+			NAPI_GRO_CB(skb)->flush = 1;
+			return NULL;
+		}
 
+		/* skb->csum is invalid if frame is padded */
+		skb->ip_summed = CHECKSUM_NONE;
 	case CHECKSUM_NONE:
 		wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr,
-					  skb_gro_len(skb), IPPROTO_TCP, 0);
+					  length, IPPROTO_TCP, 0);
 		sum = csum_fold(skb_checksum(skb,
 					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
+					     length,
 					     wsum));
 		if (sum)
 			goto flush;
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index c1097c7..f6047cc 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -36,27 +36,38 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
 					 struct sk_buff *skb)
 {
 	const struct ipv6hdr *iph = skb_gro_network_header(skb);
+	int length = ntohs(iph->payload_len);
 	__wsum wsum;
 	__sum16 sum;
 
+	/* adjust for any offset due to extension headers */
+	length += skb_transport_offset(skb) - skb_gro_offset(skb);
+
+	/* verify the entire packet is here */
+	if (length > skb_gro_len(skb))
+		goto flush;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
-		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
-				  skb->csum)) {
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
-			break;
-		}
+		if (length == skb_gro_len(skb)) {
+			if (!tcp_v6_check(length, &iph->saddr, &iph->daddr,
+					  skb->csum)) {
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
+				break;
+			}
 flush:
-		NAPI_GRO_CB(skb)->flush = 1;
-		return NULL;
+			NAPI_GRO_CB(skb)->flush = 1;
+			return NULL;
+		}
 
+		/* skb->csum is invalid if frame is padded */
+		skb->ip_summed = CHECKSUM_NONE;
 	case CHECKSUM_NONE:
 		wsum = ~csum_unfold(csum_ipv6_magic(&iph->saddr, &iph->daddr,
-						    skb_gro_len(skb),
-						    IPPROTO_TCP, 0));
+						    length, IPPROTO_TCP, 0));
 		sum = csum_fold(skb_checksum(skb,
 					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
+					     length,
 					     wsum));
 		if (sum)
 			goto flush;

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html