Re: [PATCH net] ipv6: ip6_fragment: fix headroom tests and skb leak

[David Woodhouse]

On Wed, 2015-09-16 at 17:26 +0200, Florian Westphal wrote:
> I tested this e1000 driver hacked to not allocate additional headroom
> (we end up in slowpath, since LL_RESERVED_SPACE is 16).

And it works on the originally-offending setup too; thanks.

Tested-by: David Woodhouse 

> Reported-by: David Woodhouse 
> Diagnosed-by: David Woodhouse 

They generally prefer me to use @intel.com for those too, if you would.
I draw the line at using it for actual email communication though :)

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH net] ipv6: ip6_fragment: fix headroom tests and skb leak

[David Miller]

From: Florian Westphal 
Date: Wed, 16 Sep 2015 17:26:14 +0200

> 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
> [..]
> ip6_finish_output2+0x196/0x4da
> 
> 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
> fragments).
> 
> Reported-by: David Woodhouse 
> Diagnosed-by: David Woodhouse 
> Signed-off-by: Florian Westphal 

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

[PATCH net] ipv6: ip6_fragment: fix headroom tests and skb leak

[Florian Westphal]

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
[..]
ip6_finish_output2+0x196/0x4da

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
fragments).

Reported-by: David Woodhouse 
Diagnosed-by: David Woodhouse 
Signed-off-by: Florian Westphal 
---
 net/ipv6/ip6_output.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 26ea479..92b1aa3 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -586,20 +586,22 @@ int ip6_fragment(struct sock *sk, struct sk_buff *skb,
frag_id = ipv6_select_ident(net, _hdr(skb)->daddr,
_hdr(skb)->saddr);
 
+   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? */
@@ -616,8 +618,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);
/* BUILD HEADER */
 
*prevhdr = NEXTHDR_FRAGMENT;
@@ -625,8 +625,11 @@ int ip6_fragment(struct sock *sk, struct sk_buff *skb,
if (!tmp_hdr) {
IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
  IPSTATS_MIB_FRAGFAILS);
-   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));
@@ -723,7 +726,6 @@ slow_path:
 */
 
*prevhdr = NEXTHDR_FRAGMENT;
-   hroom = LL_RESERVED_SPACE(rt->dst.dev);
troom = rt->dst.dev->needed_tailroom;
 
/*
-- 
2.0.5

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