[Devel] [PATCH RHEL7 COMMIT] ms/ipv6: ip6_fragment: fix headroom tests and skb leak

[Konstantin Khorenko]

Please consider to create a ReadyKernel patch for this.

https://readykernel.com/

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 07/13/2017 01:49 PM, Konstantin Khorenko wrote:
> The commit is pushed to "branch-rh7-3.10.0-514.26.1.vz7.33.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
> after rh7-3.10.0-514.26.1.vz7.33.5
> ------>
> commit 7054435a2d1ad20bbbd7861cb57d69a9a460c740
> Author: Florian Westphal <fw at strlen.de>
> Date:   Thu Jul 13 14:49:09 2017 +0400
>
>     ms/ipv6: ip6_fragment: fix headroom tests and skb leak
>
>     ML commit 1d325d217c7f190a42fb620ead20bb240fc16af0
>
>         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
>         [..]
>         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 <dwmw2 at infradead.org>
>         Diagnosed-by: David Woodhouse <dwmw2 at infradead.org>
>         Signed-off-by: Florian Westphal <fw at strlen.de>
>         Tested-by: David Woodhouse <David.Woodhouse at intel.com>
>         Signed-off-by: David S. Miller <davem at 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
>
>     https://jira.sw.ru/browse/PSBM-68362
>     https://bugs.openvz.org/browse/OVZ-6915
>
>     Signed-off-by: Vasily Averin <vvs at virtuozzo.com>
> ---
>  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 6b3e570..11e64fb 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -589,20 +589,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? */
> @@ -619,8 +621,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;
> @@ -628,8 +628,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));
> @@ -725,7 +728,6 @@ int ip6_fragment(struct sock *sk, struct sk_buff *skb,
>  	 *	Fragment the datagram.
>  	 */
>
> -	hroom = LL_RESERVED_SPACE(rt->dst.dev);
>  	troom = rt->dst.dev->needed_tailroom;
>
>  	/*
> .
>