[PATCHv3 net 1/3] openvswitch: Fix double-free on ip_defrag() errors

[PATCHv3 net 1/3] openvswitch: Fix double-free on ip_defrag() errors

[Joe Stringer]

If ip_defrag() returns an error other than -EINPROGRESS, then the skb is
freed. When handle_fragments() passes this back up to
do_execute_actions(), it will be freed again. Prevent this double free
by never freeing the skb in do_execute_actions() for errors returned by
ovs_ct_execute. Always free it in ovs_ct_execute() error paths instead.

Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
Reported-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
v2: Remove extraneous whitespace change.
v3: Push skb before checking error to free
---
 net/openvswitch/actions.c   |  4 ++--
 net/openvswitch/conntrack.c | 17 +++++++++++++----
 net/openvswitch/conntrack.h |  1 +
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 0bf0f406de52..dba635d086b2 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1109,8 +1109,8 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 					     nla_data(a));
 
 			/* Hide stolen IP fragments from user space. */
-			if (err == -EINPROGRESS)
-				return 0;
+			if (err)
+				return err == -EINPROGRESS ? 0 : err;
 			break;
 		}
 
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index a5ec34f8502f..b5dcc0abde66 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -293,6 +293,9 @@ static int ovs_ct_helper(struct sk_buff *skb, u16 proto)
 	return helper->help(skb, protoff, ct, ctinfo);
 }
 
+/* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
+ * value if 'skb' is freed.
+ */
 static int handle_fragments(struct net *net, struct sw_flow_key *key,
 			    u16 zone, struct sk_buff *skb)
 {
@@ -308,8 +311,8 @@ static int handle_fragments(struct net *net, struct sw_flow_key *key,
 			return err;
 
 		ovs_cb.mru = IPCB(skb)->frag_max_size;
-	} else if (key->eth.type == htons(ETH_P_IPV6)) {
 #if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
+	} else if (key->eth.type == htons(ETH_P_IPV6)) {
 		enum ip6_defrag_users user = IP6_DEFRAG_CONNTRACK_IN + zone;
 		struct sk_buff *reasm;
 
@@ -318,17 +321,18 @@ static int handle_fragments(struct net *net, struct sw_flow_key *key,
 		if (!reasm)
 			return -EINPROGRESS;
 
-		if (skb == reasm)
+		if (skb == reasm) {
+			kfree_skb(skb);
 			return -EINVAL;
+		}
 
 		key->ip.proto = ipv6_hdr(reasm)->nexthdr;
 		skb_morph(skb, reasm);
 		consume_skb(reasm);
 		ovs_cb.mru = IP6CB(skb)->frag_max_size;
-#else
-		return -EPFNOSUPPORT;
 #endif
 	} else {
+		kfree_skb(skb);
 		return -EPFNOSUPPORT;
 	}
 
@@ -473,6 +477,9 @@ static bool labels_nonzero(const struct ovs_key_ct_labels *labels)
 	return false;
 }
 
+/* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
+ * value if 'skb' is freed.
+ */
 int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 		   struct sw_flow_key *key,
 		   const struct ovs_conntrack_info *info)
@@ -508,6 +515,8 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 					&info->labels.mask);
 err:
 	skb_push(skb, nh_ofs);
+	if (err)
+		kfree_skb(skb);
 	return err;
 }
 
diff --git a/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h
index 82e0dfc66028..a7544f405c16 100644
--- a/net/openvswitch/conntrack.h
+++ b/net/openvswitch/conntrack.h
@@ -67,6 +67,7 @@ static inline int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 				 struct sw_flow_key *key,
 				 const struct ovs_conntrack_info *info)
 {
+	kfree_skb(skb);
 	return -ENOTSUPP;
 }
 
-- 
2.1.4

[PATCHv3 net 2/3] ipv6: Export nf_ct_frag6_consume_orig()

[Joe Stringer]

This is needed in openvswitch to fix an skb leak in the next patch.

Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
v2-v3: No change.
---
 net/ipv6/netfilter/nf_conntrack_reasm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 701cd2bae0a9..c7196ad1d69f 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -646,6 +646,7 @@ void nf_ct_frag6_consume_orig(struct sk_buff *skb)
 		s = s2;
 	}
 }
+EXPORT_SYMBOL_GPL(nf_ct_frag6_consume_orig);
 
 static int nf_ct_net_init(struct net *net)
 {
-- 
2.1.4

Re: [PATCHv3 net 2/3] ipv6: Export nf_ct_frag6_consume_orig()

[Pravin Shelar]

On Sun, Oct 25, 2015 at 8:21 PM, Joe Stringer <joestringer@nicira.com> wrote:
> This is needed in openvswitch to fix an skb leak in the next patch.
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCHv3 net 2/3] ipv6: Export nf_ct_frag6_consume_orig()

[David Miller]

From: Joe Stringer <joestringer@nicira.com>
Date: Sun, 25 Oct 2015 20:21:49 -0700

> This is needed in openvswitch to fix an skb leak in the next patch.
> 
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Applied.

[PATCHv3 net 3/3] openvswitch: Fix skb leak using IPv6 defrag

[Joe Stringer]

nf_ct_frag6_gather() makes a clone of each skb passed to it, and if the
reassembly is successful, expects the caller to free all of the original
skbs using nf_ct_frag6_consume_orig(). This call was previously missing,
meaning that the original fragments were never freed (with the exception
of the last fragment to arrive).

Fix this by ensuring that all original fragments except for the last
fragment are freed via nf_ct_frag6_consume_orig(). The last fragment
will be morphed into the head, so it must not be freed yet. Furthermore,
retain the ->next pointer for the head after skb_morph().

Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
Reported-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
v2-v3: No change.
---
 net/openvswitch/conntrack.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index b5dcc0abde66..50095820edb7 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -326,8 +326,15 @@ static int handle_fragments(struct net *net, struct sw_flow_key *key,
 			return -EINVAL;
 		}
 
+		/* Don't free 'skb' even though it is one of the original
+		 * fragments, as we're going to morph it into the head.
+		 */
+		skb_get(skb);
+		nf_ct_frag6_consume_orig(reasm);
+
 		key->ip.proto = ipv6_hdr(reasm)->nexthdr;
 		skb_morph(skb, reasm);
+		skb->next = reasm->next;
 		consume_skb(reasm);
 		ovs_cb.mru = IP6CB(skb)->frag_max_size;
 #endif
-- 
2.1.4

Re: [PATCHv3 net 3/3] openvswitch: Fix skb leak using IPv6 defrag

[Pravin Shelar]

On Sun, Oct 25, 2015 at 8:21 PM, Joe Stringer <joestringer@nicira.com> wrote:
> nf_ct_frag6_gather() makes a clone of each skb passed to it, and if the
> reassembly is successful, expects the caller to free all of the original
> skbs using nf_ct_frag6_consume_orig(). This call was previously missing,
> meaning that the original fragments were never freed (with the exception
> of the last fragment to arrive).
>
> Fix this by ensuring that all original fragments except for the last
> fragment are freed via nf_ct_frag6_consume_orig(). The last fragment
> will be morphed into the head, so it must not be freed yet. Furthermore,
> retain the ->next pointer for the head after skb_morph().
>
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Reported-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCHv3 net 3/3] openvswitch: Fix skb leak using IPv6 defrag

[David Miller]

From: Joe Stringer <joestringer@nicira.com>
Date: Sun, 25 Oct 2015 20:21:50 -0700

> nf_ct_frag6_gather() makes a clone of each skb passed to it, and if the
> reassembly is successful, expects the caller to free all of the original
> skbs using nf_ct_frag6_consume_orig(). This call was previously missing,
> meaning that the original fragments were never freed (with the exception
> of the last fragment to arrive).
> 
> Fix this by ensuring that all original fragments except for the last
> fragment are freed via nf_ct_frag6_consume_orig(). The last fragment
> will be morphed into the head, so it must not be freed yet. Furthermore,
> retain the ->next pointer for the head after skb_morph().
> 
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Reported-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Applied.

Re: [PATCHv3 net 1/3] openvswitch: Fix double-free on ip_defrag() errors

[Pravin Shelar]

On Sun, Oct 25, 2015 at 8:21 PM, Joe Stringer <joestringer@nicira.com> wrote:
> If ip_defrag() returns an error other than -EINPROGRESS, then the skb is
> freed. When handle_fragments() passes this back up to
> do_execute_actions(), it will be freed again. Prevent this double free
> by never freeing the skb in do_execute_actions() for errors returned by
> ovs_ct_execute. Always free it in ovs_ct_execute() error paths instead.
>
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Reported-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCHv3 net 1/3] openvswitch: Fix double-free on ip_defrag() errors

[David Miller]

From: Joe Stringer <joestringer@nicira.com>
Date: Sun, 25 Oct 2015 20:21:48 -0700

> If ip_defrag() returns an error other than -EINPROGRESS, then the skb is
> freed. When handle_fragments() passes this back up to
> do_execute_actions(), it will be freed again. Prevent this double free
> by never freeing the skb in do_execute_actions() for errors returned by
> ovs_ct_execute. Always free it in ovs_ct_execute() error paths instead.
> 
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Reported-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Applied.