[PATCH v2] net: Do not include padding in TCP GRO checksum

[PATCH v2] net: Do not include padding in TCP GRO checksum

[Alexander Duyck]

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@intel.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.

 net/ipv4/tcp_offload.c   |   26 +++++++++++++++++---------
 net/ipv6/tcpv6_offload.c |   27 +++++++++++++++++----------
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index a2b68a1..17b5d14 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -273,26 +273,34 @@ 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);
+
 	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..abfa938 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -36,27 +36,34 @@ 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);
+
 	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;

Re: [PATCH v2] net: Do not include padding in TCP GRO checksum

[Eric Dumazet]

On Fri, 2013-11-15 at 12:03 -0800, Alexander Duyck wrote:
> 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@intel.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.
> 
>  net/ipv4/tcp_offload.c   |   26 +++++++++++++++++---------
>  net/ipv6/tcpv6_offload.c |   27 +++++++++++++++++----------
>  2 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index a2b68a1..17b5d14 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -273,26 +273,34 @@ 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);


Sorry, but couldn't iph->tot_len be larger than skb_gro_len(skb) ?

It's not validated in inet_gro_complete() ...

skb_checksum(skb, ..., length, wsum)  could crash.