[PATCH] Fix corruption of skb csum field in pskb_expand_head() of net/core/skbuff.c

[PATCH] Fix corruption of skb csum field in pskb_expand_head() of net/core/skbuff.c

[Andrea Shepard]

Make pskb_expand_head() check ip_summed to make sure csum_start is really
csum_start and not csum before adjusting it.

This fixes a bug I encountered using a Sun Quad-Fast Ethernet card and VLANs.
On my configuration, the sunhme driver produces skbs with differing amounts
of headroom on receive depending on the packet size.  See line 2030 of
drivers/net/sunhme.c; packets smaller than RX_COPY_THRESHOLD have 52 bytes
of headroom but packets larger than that cutoff have only 20 bytes.

When these packets reach the VLAN driver, vlan_check_reorder_header()
calls skb_cow(), which, if the packet has less than NET_SKB_PAD (== 32) bytes
of headroom, uses pskb_expand_head() to make more.

Then, pskb_expand_head() needs to adjust a lot of offsets into the skb,
including csum_start.  Since csum_start is a union with csum, if the packet
has a valid csum value this will corrupt it, which was the effect I observed.
The sunhme hardware computes receive checksums, so the skbs would be created
by the driver with ip_summed == CHECKSUM_COMPLETE and a valid csum field, and
then pskb_expand_head() would corrupt the csum field, leading to an "hw csum
error" message later on, for example in icmp_rcv() for pings larger than the
sunhme RX_COPY_THRESHOLD.

On the basis of the comment at the beginning of include/linux/skbuff.h,
I believe that the csum_start skb field is only meaningful if ip_csummed is
CSUM_PARTIAL, so this patch makes pskb_expand_head() adjust it only in that
case to avoid corrupting a valid csum value.

Please see my more in-depth disucssion of tracking down this bug for
more details if you like:

http://puellavulnerata.livejournal.com/112186.html
http://puellavulnerata.livejournal.com/112567.html
http://puellavulnerata.livejournal.com/112891.html
http://puellavulnerata.livejournal.com/113096.html
http://puellavulnerata.livejournal.com/113591.html

I am not subscribed to this list, so please CC me on replies.

Signed-off-by: Andrea Shepard <andrea@persephoneslair.org>

diff -Nau linux-2.6.34.1/net/core/skbuff.c linux-2.6.34.1-skb-csum/net/core/skbuff.c
--- linux-2.6.34.1/net/core/skbuff.c    2010-07-05 11:24:10.000000000 -0700
+++ linux-2.6.34.1-skb-csum/net/core/skbuff.c   2010-07-21 18:42:33.000000000 -0700
@@ -854,7 +854,9 @@
        skb->network_header   += off;
        if (skb_mac_header_was_set(skb))
                skb->mac_header += off;
-       skb->csum_start       += nhead;
+       /* Only adjust this if it actually is csum_start rather than csum */
+       if (skb->ip_summed == CHECKSUM_PARTIAL)
+               skb->csum_start += nhead;
        skb->cloned   = 0;
        skb->hdr_len  = 0;
        skb->nohdr    = 0;

-- 
Andrea Shepard

Re: [PATCH] Fix corruption of skb csum field in pskb_expand_head() of net/core/skbuff.c

[David Miller]

From: Andrea Shepard <andrea@persephoneslair.org>
Date: Thu, 22 Jul 2010 12:12:35 -0700

> Make pskb_expand_head() check ip_summed to make sure csum_start is really
> csum_start and not csum before adjusting it.
...
> Signed-off-by: Andrea Shepard <andrea@persephoneslair.org>

Applied, but your email client corrupted tab characters into spaces so
I had to apply your patch by hand.

There is a similar bug in skb_copy_expand() so I fixed that too.

Thanks again.

--------------------
net: Fix skb_copy_expand() handling of ->csum_start

It should only be adjusted if ip_summed == CHECKSUM_PARTIAL.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/core/skbuff.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index c699159..ce88293 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -932,7 +932,8 @@ struct sk_buff *skb_copy_expand(const struct sk_buff *skb,
 	copy_skb_header(n, skb);
 
 	off                  = newheadroom - oldheadroom;
-	n->csum_start       += off;
+	if (n->ip_summed == CHECKSUM_PARTIAL)
+		n->csum_start += off;
 #ifdef NET_SKBUFF_DATA_USES_OFFSET
 	n->transport_header += off;
 	n->network_header   += off;
-- 
1.7.1.1

[PATCH net-next-2.6] net: pskb_expand_head() optimization

[Eric Dumazet]

Le jeudi 22 juillet 2010 à 12:12 -0700, Andrea Shepard a écrit :
> Make pskb_expand_head() check ip_summed to make sure csum_start is really
> csum_start and not csum before adjusting it.
> 
> This fixes a bug I encountered using a Sun Quad-Fast Ethernet card and VLANs.
> On my configuration, the sunhme driver produces skbs with differing amounts
> of headroom on receive depending on the packet size.  See line 2030 of
> drivers/net/sunhme.c; packets smaller than RX_COPY_THRESHOLD have 52 bytes
> of headroom but packets larger than that cutoff have only 20 bytes.
> 
> When these packets reach the VLAN driver, vlan_check_reorder_header()
> calls skb_cow(), which, if the packet has less than NET_SKB_PAD (== 32) bytes
> of headroom, uses pskb_expand_head() to make more.
> 
> Then, pskb_expand_head() needs to adjust a lot of offsets into the skb,
> including csum_start.  Since csum_start is a union with csum, if the packet
> has a valid csum value this will corrupt it, which was the effect I observed.
> The sunhme hardware computes receive checksums, so the skbs would be created
> by the driver with ip_summed == CHECKSUM_COMPLETE and a valid csum field, and
> then pskb_expand_head() would corrupt the csum field, leading to an "hw csum
> error" message later on, for example in icmp_rcv() for pings larger than the
> sunhme RX_COPY_THRESHOLD.
> 
> On the basis of the comment at the beginning of include/linux/skbuff.h,
> I believe that the csum_start skb field is only meaningful if ip_csummed is
> CSUM_PARTIAL, so this patch makes pskb_expand_head() adjust it only in that
> case to avoid corrupting a valid csum value.
> 
> Please see my more in-depth disucssion of tracking down this bug for
> more details if you like:
> 
> http://puellavulnerata.livejournal.com/112186.html
> http://puellavulnerata.livejournal.com/112567.html
> http://puellavulnerata.livejournal.com/112891.html
> http://puellavulnerata.livejournal.com/113096.html
> http://puellavulnerata.livejournal.com/113591.html
> 
> I am not subscribed to this list, so please CC me on replies.
> 

Excellent Changelog Andrea, thanks !

Reviewing pskb_expand_head(), I see it copy the whole struct
skb_shared_info, even if source contains garbage (uninitialized data).

I wonder why it is not triggering kmemcheck alarms

[PATCH net-next-2.6] net: pskb_expand_head() optimization

Move frags[] at the end of struct skb_shared_info, and make
pskb_expand_head() copy only the used part of it instead of whole array.

This should avoid kmemcheck warnings and speedup pskb_expand_head() as
well, avoiding a lot of cache misses.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/skbuff.h |    3 ++-
 net/core/skbuff.c      |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f5aa87e..d89876b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -202,10 +202,11 @@ struct skb_shared_info {
 	 */
 	atomic_t	dataref;
 
-	skb_frag_t	frags[MAX_SKB_FRAGS];
 	/* Intermediate layers must ensure that destructor_arg
 	 * remains valid until skb destructor */
 	void *		destructor_arg;
+	/* must be last field, see pskb_expand_head() */
+	skb_frag_t	frags[MAX_SKB_FRAGS];
 };
 
 /* We divide dataref into two halves.  The higher 16 bits hold references
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 76d33ca..7da58a2 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -817,7 +817,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	memcpy(data + nhead, skb->head, skb->tail - skb->head);
 #endif
 	memcpy(data + size, skb_end_pointer(skb),
-	       sizeof(struct skb_shared_info));
+	       offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags]));
 
 	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
 		get_page(skb_shinfo(skb)->frags[i].page);

Re: [PATCH net-next-2.6] net: pskb_expand_head() optimization

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 23 Jul 2010 07:09:08 +0200

> [PATCH net-next-2.6] net: pskb_expand_head() optimization
> 
> Move frags[] at the end of struct skb_shared_info, and make
> pskb_expand_head() copy only the used part of it instead of whole array.
> 
> This should avoid kmemcheck warnings and speedup pskb_expand_head() as
> well, avoiding a lot of cache misses.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Maybe it's just that people aren't running kmemcheck when a
pskb_expand_head() triggers, who knows.

Anyways, since we skip the ->frag[] array in skb alloc, etc.,
your patch is of course fine.

Applied, thanks!