[Devel,vz7] ipv6: ip6_fragment: fix headroom tests and skb leak

Submitted by Vasily Averin on July 13, 2017, 7:47 a.m.


Message ID 008138d2-50c5-2751-395a-fc0cd5e39802@virtuozzo.com
State New
Series "ipv6: ip6_fragment: fix headroom tests and skb leak"
Headers show

Commit Message

Vasily Averin July 13, 2017, 7:47 a.m.
ML commit 1d325d217c7f190a42fb620ead20bb240fc16af0
Author: Florian Westphal <fw@strlen.de>
Date:   Wed Sep 16 17:26:14 2015 +0200

    ipv6: ip6_fragment: fix headroom tests and skb leak
    David Woodhouse reports skb_under_panic when we try to push ethernet
    header to fragmented ipv6 skbs:
     skbuff: skb_under_panic: text:c1277f1e len:1294 put:14 head:dec98000
     data:dec97ffc tail:0xdec9850a end:0xdec98f40 dev:br-lan
    David further debugged this:
      [..] offending fragments were arriving here with skb_headroom(skb)==10.
      Which is reasonable, being the Solos ADSL card's header of 8 bytes
      followed by 2 bytes of PPP frame type.
    The problem is that if netfilter ipv6 defragmentation is used, skb_cow()
    in ip6_forward will only see reassembled skb.
    Therefore, headroom is overestimated by 8 bytes (we pulled fragment
    header) and we don't check the skbs in the frag_list either.
    We can't do these checks in netfilter defrag since outdev isn't known yet.
    Furthermore, existing tests in ip6_fragment did not consider the fragment
    or ipv6 header size when checking headroom of the fraglist skbs.
    While at it, also fix a skb leak on memory allocation -- ip6_fragment
    must consume the skb.
    I tested this e1000 driver hacked to not allocate additional headroom
    (we end up in slowpath, since LL_RESERVED_SPACE is 16).
    If 2 bytes of headroom are allocated, fastpath is taken (14 byte
    ethernet header was pulled, so 16 byte headroom available in all
    Reported-by: David Woodhouse <dwmw2@infradead.org>
    Diagnosed-by: David Woodhouse <dwmw2@infradead.org>
    Signed-off-by: Florian Westphal <fw@strlen.de>
    Tested-by: David Woodhouse <David.Woodhouse@intel.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

VvS: in our case fragmented ipv6 skb was forwarded from tun6to4 interface with hard_header_len = 0 
to tap_softether with hard_header_len = 14 and triggered BUG in ip6_finish_output2
skb_under_panic: text:ffffffffa02cc57e len:290 put:14 head:ffff883981bc9c00 data:ffff883981bc9bf2 tail:0x114 end:0x140 dev:tap_softether


Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Patch hide | download patch | download mbox

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index dc632eb..8cd86a2 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -590,20 +590,22 @@  int ip6_fragment(struct sock *sk, struct sk_buff *skb,
 	    (err = skb_checksum_help(skb)))
 		goto fail;
+	hroom = LL_RESERVED_SPACE(rt->dst.dev);
 	if (skb_has_frag_list(skb)) {
 		int first_len = skb_pagelen(skb);
 		struct sk_buff *frag2;
 		if (first_len - hlen > mtu ||
 		    ((first_len - hlen) & 7) ||
-		    skb_cloned(skb))
+		    skb_cloned(skb) ||
+		    skb_headroom(skb) < (hroom + sizeof(struct frag_hdr)))
 			goto slow_path;
 		skb_walk_frags(skb, frag) {
 			/* Correct geometry. */
 			if (frag->len > mtu ||
 			    ((frag->len & 7) && frag->next) ||
-			    skb_headroom(frag) < hlen)
+			    skb_headroom(frag) < (hlen + hroom + sizeof(struct frag_hdr)))
 				goto slow_path_clean;
 			/* Partially cloned skb? */
@@ -620,8 +622,6 @@  int ip6_fragment(struct sock *sk, struct sk_buff *skb,
 		err = 0;
 		offset = 0;
-		frag = skb_shinfo(skb)->frag_list;
-		skb_frag_list_init(skb);
 		*prevhdr = NEXTHDR_FRAGMENT;
@@ -629,8 +629,11 @@  int ip6_fragment(struct sock *sk, struct sk_buff *skb,
 		if (!tmp_hdr) {
 			IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
-			return -ENOMEM;
+			err = -ENOMEM;
+			goto fail;
+		frag = skb_shinfo(skb)->frag_list;
+		skb_frag_list_init(skb);
 		__skb_pull(skb, hlen);
 		fh = (struct frag_hdr*)__skb_push(skb, sizeof(struct frag_hdr));
@@ -727,7 +730,6 @@  int ip6_fragment(struct sock *sk, struct sk_buff *skb,
 	*prevhdr = NEXTHDR_FRAGMENT;
-	hroom = LL_RESERVED_SPACE(rt->dst.dev);
 	troom = rt->dst.dev->needed_tailroom;