From: Ben Pfaff <blp@nicira.com>
Date: Thu, 13 Nov 2008 19:29:20 +0000 (-0800)
Subject: Fix double-free: NF_HOOK sometimes frees the sk_buff passed in.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ad27c8ac82007a09fbb302c0993b2c8746311a3d;p=openvswitch

Fix double-free: NF_HOOK sometimes frees the sk_buff passed in.

NF_HOOK is supposed to *always* consume the sk_buff passed in, either
internally or through the okfn argument.  We assumed that it never
consumed its sk_buff, which was OK in the case where it called okfn,
since our okfn (snat_pre_route_finish) never freed its sk_buff, but
not when one of the netfilter hooks dropped or stole the packet, because
then we'd assume that it still existed and free it a second time.

The other users of NF_HOOK in this file, in snat_skb() and
snat_skb_finish(), do not need to be fixed because they always pass a
copy of their sk_buff argument to NF_HOOK and expect it to be freed.
---

diff --git a/datapath/datapath.c b/datapath/datapath.c
index bca1bdc5..4f5acd03 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -466,7 +466,6 @@ do_port_input(struct net_bridge_port *p, struct sk_buff *skb)
 #ifdef SUPPORT_SNAT
 	/* Check if this packet needs early SNAT processing. */
 	if (snat_pre_route(skb)) {
-		kfree_skb(skb);
 		return;
 	}
 #endif
diff --git a/datapath/nx_act_snat.c b/datapath/nx_act_snat.c
index f846671f..d3750f1b 100644
--- a/datapath/nx_act_snat.c
+++ b/datapath/nx_act_snat.c
@@ -179,18 +179,22 @@ snat_pre_route_finish(struct sk_buff *skb)
 	/* Don't process packets that were not translated due to NAT */
 	spin_lock_irqsave(&p->lock, flags);
 	sc = p->snat;
-	if (__snat_this_address(sc, iph->daddr)) {
-		spin_unlock_irqrestore(&p->lock, flags);
-		return -1;
-	}
-
-	/* If SNAT is configured for this input device, check the IP->MAC
-	 * mappings to see if we should update the destination MAC. */
-	if (sc)
-		dnat_mac(skb->dev->br_port, skb);
+	if (!__snat_this_address(sc, iph->daddr)) {
+		/* If SNAT is configured for this input device, check the
+		 * IP->MAC mappings to see if we should update the destination
+		 * MAC. */
+		if (sc)
+			dnat_mac(skb->dev->br_port, skb);
 
+	}
 	spin_unlock_irqrestore(&p->lock, flags);
 
+	/* Pass the translated packet as input to the OpenFlow stack, which
+	 * consumes it. */
+	skb_push(skb, ETH_HLEN);
+	skb_reset_mac_header(skb);
+	fwd_port_input(p->dp->chain, skb, p);
+
 	return 0;
 }
 
@@ -305,9 +309,8 @@ handle_icmp_snat(struct sk_buff *skb)
  * modification based on prior SNAT action and responding to ARP and
  * echo requests for the SNAT interface. 
  *
- * Returns 0 if 'skb' should continue to be processed by the caller.
- * Returns -1 if the packet was handled, and the caller should free
- * 'skb'.
+ * Returns -1 if the packet was handled and consumed, 0 if the caller
+ * should continue to process 'skb'.
  */
 int
 snat_pre_route(struct sk_buff *skb)
@@ -316,42 +319,47 @@ snat_pre_route(struct sk_buff *skb)
 	int len;
 
 	WARN_ON_ONCE(skb_network_offset(skb));
-	if (skb->protocol == htons(ETH_P_ARP)) 
-		return handle_arp_snat(skb);
+	if (skb->protocol == htons(ETH_P_ARP)) {
+		if (handle_arp_snat(skb))
+			goto consume;
+		return 0;
+	}
 	else if (skb->protocol != htons(ETH_P_IP)) 
 		return 0;
 
 	if (!pskb_may_pull(skb, sizeof *iph))
-		goto ipv4_error;
+		goto consume;
 
 	iph = ip_hdr(skb);
 	if (iph->ihl < 5 || iph->version != 4)
-		goto ipv4_error;
+		goto consume;
 
 	if (!pskb_may_pull(skb, ip_hdrlen(skb)))
-		goto ipv4_error;
+		goto consume;
 	skb_set_transport_header(skb, ip_hdrlen(skb));
 
 	/* Check if we need to echo reply for this address */
 	iph = ip_hdr(skb);
 	if ((iph->protocol == IPPROTO_ICMP) && (handle_icmp_snat(skb))) 
-		return -1;
+		goto consume;
 
 	iph = ip_hdr(skb);
 	if (unlikely(ip_fast_csum(iph, iph->ihl)))
-		goto ipv4_error;
+		goto consume;
 
 	len = ntohs(iph->tot_len);
 	if ((skb->len < len) || len < (iph->ihl*4))
-		goto ipv4_error;
+		goto consume;
 
 	if (pskb_trim_rcsum(skb, len))
-		goto ipv4_error;
+		goto consume;
 
-	return NF_HOOK(PF_INET, NF_INET_PRE_ROUTING, skb, skb->dev, NULL,
-			snat_pre_route_finish);
+	NF_HOOK(PF_INET, NF_INET_PRE_ROUTING, skb, skb->dev, NULL,
+		snat_pre_route_finish);
+	return -1;
 
-ipv4_error:
+consume:
+	kfree_skb(skb);
 	return -1;
 }