Skip to contentSkip to conversation

ip multicast route bug fix

PATCH Applied Stephen Hemminger 6 participants 14 messages Jul 24, 2006 – Jul 26, 2006

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

PATCH 1/1 ip multicast route bug fix

This should fix the problem reported in http://bugzilla.kernel.org/show_bug.cgi?id=6186
where the skb is used after freed. The code in IP multicast route.

Code was reusing an skb which could lead to use after free or double free.

Signed-off-by Stephen Hemminger <[email protected]>
---
 net/ipv4/ipmr.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)
net/ipv4/ipmr.c +14 -6
index ba33f86..d336104 100644
@@ -1580,6 +1580,7 @@ int ipmr_get_route(struct sk_buff *skb, 
 	cache = ipmr_cache_find(rt->rt_src, rt->rt_dst);
 
 	if (cache==NULL) {
+		struct sk_buff *iskb;
 		struct net_device *dev;
 		int vif;
 

@@ -1593,12 +1594,19 @@ int ipmr_get_route(struct sk_buff *skb, 
 			read_unlock(&mrt_lock);
 			return -ENODEV;
 		}
-		skb->nh.raw = skb_push(skb, sizeof(struct iphdr));
-		skb->nh.iph->ihl = sizeof(struct iphdr)>>2;
-		skb->nh.iph->saddr = rt->rt_src;
-		skb->nh.iph->daddr = rt->rt_dst;
-		skb->nh.iph->version = 0;
-		err = ipmr_cache_unresolved(vif, skb);
+		
+		iskb = alloc_skb(sizeof(struct iphdr), GFP_KERNEL);
+		if (!iskb) {
+			read_unlock(&mrt_lock);
+			return -ENOMEM;
+		}
+		memset(iskb->data, 0, sizeof(struct iphdr));
+		iskb->nh.raw = iskb->data;
+		iskb->nh.iph->ihl = sizeof(struct iphdr)>>2;
+		iskb->nh.iph->saddr = rt->rt_src;
+		iskb->nh.iph->daddr = rt->rt_dst;
+
+		err = ipmr_cache_unresolved(vif, iskb);
 		read_unlock(&mrt_lock);
 		return err;
 	}
Signature · 1 line
1.4.0
#2 lore raw
In reply to Stephen Hemminger

net/ipv4/ipmr.c int ipmr_get_route(struct sk_buff *skb, · line 1602

Quoted 11 lines · Stephen Hemminger

This should fix the problem reported in http://bugzilla.kernel.org/show_bug.cgi?id=6186
where the skb is used after freed. The code in IP multicast route.

Code was reusing an skb which could lead to use after free or double free.

net/ipv4/ipmr.c +6 -0
+		
+		iskb = alloc_skb(sizeof(struct iphdr), GFP_KERNEL);
+		if (!iskb) {
+			read_unlock(&mrt_lock);
+			return -ENOMEM;
+		}

GFP_KERNEL allocation under read_lock()?

Signature · 1 line
/Martin
#3 lore raw
In reply to Stephen Hemminger
Quoted 14 lines · Stephen Hemminger
net/ipv4/ipmr.c +3 -6
@@ -1593,12 +1594,19 @@ int ipmr_get_route(struct sk_buff *skb, 
                       read_unlock(&mrt_lock);
                       return -ENODEV;
               }
-               skb->nh.raw = skb_push(skb, sizeof(struct iphdr));
-               skb->nh.iph->ihl = sizeof(struct iphdr)>>2;
-               skb->nh.iph->saddr = rt->rt_src;
-               skb->nh.iph->daddr = rt->rt_dst;
-               skb->nh.iph->version = 0;
-               err = ipmr_cache_unresolved(vif, skb);
+               
+               iskb = alloc_skb(sizeof(struct iphdr), GFP_KERNEL);
+               if (!iskb) {

net/ipv4/ipmr.c int ipmr_get_route(struct sk_buff *skb, · line 1609

Quoted 10 lines
net/ipv4/ipmr.c +10 -0
+                       read_unlock(&mrt_lock);
+                       return -ENOMEM;
+               }
+               memset(iskb->data, 0, sizeof(struct iphdr));
+               iskb->nh.raw = iskb->data;
+               iskb->nh.iph->ihl = sizeof(struct iphdr)>>2;
+               iskb->nh.iph->saddr = rt->rt_src;
+               iskb->nh.iph->daddr = rt->rt_dst;
+
+               err = ipmr_cache_unresolved(vif, iskb);

I'm afraid this is still broken in a different way.

If ipmr_cache_unresolved queues the skb onto the unresolved list things it's going to try to use the skb as a netlink skb instead :)

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#4 lore raw

PATCH 1/1 ip multicast route bug fix (rev2)

IP multicast route code was reusing an skb which causes use after free and double free.

Signed-off-by Stephen Hemminger <[email protected]>
---
 net/ipv4/ipmr.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)
net/ipv4/ipmr.c +16 -6
index ba33f86..f5d3d96 100644
@@ -1580,6 +1580,7 @@ int ipmr_get_route(struct sk_buff *skb, 
 	cache = ipmr_cache_find(rt->rt_src, rt->rt_dst);
 
 	if (cache==NULL) {
+		struct sk_buff *skb2;
 		struct net_device *dev;
 		int vif;
 

@@ -1593,12 +1594,21 @@ int ipmr_get_route(struct sk_buff *skb, 
 			read_unlock(&mrt_lock);
 			return -ENODEV;
 		}
-		skb->nh.raw = skb_push(skb, sizeof(struct iphdr));
-		skb->nh.iph->ihl = sizeof(struct iphdr)>>2;
-		skb->nh.iph->saddr = rt->rt_src;
-		skb->nh.iph->daddr = rt->rt_dst;
-		skb->nh.iph->version = 0;
-		err = ipmr_cache_unresolved(vif, skb);
+		
+		skb2 = alloc_skb(sizeof(struct iphdr) + sizeof(struct nlmsghdr),
+				 GFP_ATOMIC);
+		if (!skb2) {
+			read_unlock(&mrt_lock);
+			return -ENOMEM;
+		}
+
+		memset(skb2->data, 0, sizeof(struct iphdr));
+		skb2->nh.raw = skb2->data;
+		skb2->nh.iph->ihl = sizeof(struct iphdr)>>2;
+		skb2->nh.iph->saddr = rt->rt_src;
+		skb2->nh.iph->daddr = rt->rt_dst;
+
+		err = ipmr_cache_unresolved(vif, skb2);
 		read_unlock(&mrt_lock);
 		return err;
 	}
Signature · 1 line
1.4.0
#5 lore raw
In reply to Stephen Hemminger

Hello!

Quoted 1 line

Code was reusing an skb which could lead to use after free or double free.

No, this does not help. The bug is not here.

I was so ashamed of this that could not touch the thing. :-) It startled me a lot, how is it possible that the thing was in production for several years and such bad bug never was noticed?

Now it is clear. skbs leaked sometimes before BK changeset 1.889.26.53,
subj: [IPV4]: Fix skb leak in inet_rtm_getroute. But after this fix, which introduced new bug (goto out_free in the enclosed patch), the bug showed on surface.

Please, review this.

- ipmr_cache_unresolved() does not free skb. Caller does.
- goto out_free in route.c in the case, when skb is enqueued is returned to goto out.
- some cosmetic cleanup in rt_fill_info() to make it more readable.

net/ipv4/ipmr.c +18 -12
index 9ccacf5..1788c04 100644
@@ -640,8 +640,6 @@ ipmr_cache_unresolved(vifi_t vifi, struc
 		if (atomic_read(&cache_resolve_queue_len)>=10 ||
 		    (c=ipmr_cache_alloc_unres())==NULL) {
 			spin_unlock_bh(&mfc_unres_lock);
-
-			kfree_skb(skb);
 			return -ENOBUFS;
 		}
 

@@ -662,7 +660,6 @@ ipmr_cache_unresolved(vifi_t vifi, struc
 			spin_unlock_bh(&mfc_unres_lock);
 
 			kmem_cache_free(mrt_cachep, c);
-			kfree_skb(skb);
 			return err;
 		}
 

@@ -677,7 +674,6 @@ ipmr_cache_unresolved(vifi_t vifi, struc
 	 *	See if we can append the packet
 	 */
 	if (c->mfc_un.unres.unresolved.qlen>3) {
-		kfree_skb(skb);
 		err = -ENOBUFS;
 	} else {
 		skb_queue_tail(&c->mfc_un.unres.unresolved,skb);

@@ -1375,6 +1371,7 @@ int ip_mr_input(struct sk_buff *skb)
 	 */
 	if (cache==NULL) {
 		int vif;
+		int err;
 
 		if (local) {
 			struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);

@@ -1387,15 +1384,13 @@ int ip_mr_input(struct sk_buff *skb)
 		}
 
 		vif = ipmr_find_vif(skb->dev);
-		if (vif >= 0) {
-			int err = ipmr_cache_unresolved(vif, skb);
-			read_unlock(&mrt_lock);
-
-			return err;
-		}
+		err = -ENODEV;
+		if (vif >= 0)
+			err = ipmr_cache_unresolved(vif, skb);
 		read_unlock(&mrt_lock);
-		kfree_skb(skb);
-		return -ENODEV;
+		if (err)
+			kfree_skb(skb);
+		return err;
 	}
 
 	ip_mr_forward(skb, cache, local);

@@ -1568,6 +1563,17 @@ rtattr_failure:
 	return -EMSGSIZE;
 }
 
+/**
+ * ipmr_get_route - return mrouting information or queue for resolution
+ * @skb - netlink skb with partially complete rtnelink information
+ * @rtm - pointer to head of rtmsg
+ * @nowait - if mroute is not in cache, do not resolve, fail with EAGAIN
+ *
+ * Return:
+ *   1 - if mrouting information is succesfully recorded in skb
+ *   0 - if information is not available, but skb is queued for resolution
+ *   < 0 - error
+ */
 int ipmr_get_route(struct sk_buff *skb, struct rtmsg *rtm, int nowait)
 {
 	int err;
net/ipv4/route.c +6 -10
index 2dc6dbb..62f53e9 100644
@@ -2703,16 +2703,12 @@ static int rt_fill_info(struct sk_buff *
 		if (MULTICAST(dst) && !LOCAL_MCAST(dst) &&
 		    ipv4_devconf.mc_forwarding) {
 			int err = ipmr_get_route(skb, r, nowait);
-			if (err <= 0) {
-				if (!nowait) {
-					if (err == 0)
-						return 0;
+			if (err == 0)
+				return 0;
+			if (err < 0) {
+				if (err == -EMSGSIZE)
 					goto nlmsg_failure;
-				} else {
-					if (err == -EMSGSIZE)
-						goto nlmsg_failure;
-					((struct rta_cacheinfo*)RTA_DATA(eptr))->rta_error = err;
-				}
+				((struct rta_cacheinfo*)RTA_DATA(eptr))->rta_error = err;
 			}
 		} else
 #endif

@@ -2794,7 +2790,7 @@ int inet_rtm_getroute(struct sk_buff *in
 	err = rt_fill_info(skb, NETLINK_CB(in_skb).pid, nlh->nlmsg_seq,
 				RTM_NEWROUTE, 0, 0);
 	if (!err)
-		goto out_free;
+		goto out;
 	if (err < 0) {
 		err = -EMSGSIZE;
 		goto out_free;
author
#6 lore raw
Quoted 15 lines · On Tue, 25 Jul 2006 20:20:01 +0400 Alexey Kuznetsov <[email protected]>…

Hello!

Quoted 1 line

Code was reusing an skb which could lead to use after free or double free.

No, this does not help. The bug is not here.

I was so ashamed of this that could not touch the thing. :-) It startled me a lot, how is it possible that the thing was in production for several years and such bad bug never was noticed?

Now it is clear. skbs leaked sometimes before BK changeset 1.889.26.53,
subj: [IPV4]: Fix skb leak in inet_rtm_getroute. But after this fix, which introduced new bug (goto out_free in the enclosed patch), the bug showed on surface.

Quoted 6 lines

Please, review this.

- ipmr_cache_unresolved() does not free skb. Caller does.
- goto out_free in route.c in the case, when skb is enqueued is returned to goto out.
- some cosmetic cleanup in rt_fill_info() to make it more readable.

This looks correct, but it may lead to false reports from automated checking tools because the skb lifetime depends on the return value. Wouldn't it be better to have a consistent interface (skb always freed), and clone the skb if needed for deferred processing?

#7 lore raw

Hello!

Quoted 3 lines

checking tools because the skb lifetime depends on the return value. Wouldn't it be better to have a consistent interface (skb always freed), and clone the skb if needed for deferred processing?

But skb is not always freed in any case.
Normally it is submitted to netlink_unicast(). It is freed only in error case. So, following this logic should not you clone skb to feed to netlink_unicast()?

Alexey

#8 lore raw
In reply to Stephen Hemminger

Hello!

Quoted 2 lines

Wouldn't it be better to have a consistent interface (skb always freed), and clone the skb if needed for deferred processing?

I am sorry, I misunderstood you. I absolutely agree. It is much better, the variant which I suggested is a good sample of bad programming. :-)

Alexey

#9 lore raw
In reply to Stephen Hemminger

Hello!

Quoted 2 lines

Wouldn't it be better to have a consistent interface (skb always freed), and clone the skb if needed for deferred processing?

I think you mean this.

Note, it is real skb_clone(), not alloc_skb(). Equeued skb contains the whole half-prepared netlink message plus room for the rest. It could be also skb_copy(), if we want to be puristic about mangling cloned data, but original copy is really not going to be used.

Alexey

net/ipv4/ipmr.c +13 -6
index 9ccacf5..85893ee 100644
@@ -1578,6 +1578,7 @@ int ipmr_get_route(struct sk_buff *skb, 
 	cache = ipmr_cache_find(rt->rt_src, rt->rt_dst);
 
 	if (cache==NULL) {
+		struct sk_buff *skb2;
 		struct net_device *dev;
 		int vif;
 

@@ -1591,12 +1592,18 @@ int ipmr_get_route(struct sk_buff *skb, 
 			read_unlock(&mrt_lock);
 			return -ENODEV;
 		}
-		skb->nh.raw = skb_push(skb, sizeof(struct iphdr));
-		skb->nh.iph->ihl = sizeof(struct iphdr)>>2;
-		skb->nh.iph->saddr = rt->rt_src;
-		skb->nh.iph->daddr = rt->rt_dst;
-		skb->nh.iph->version = 0;
-		err = ipmr_cache_unresolved(vif, skb);
+		skb2 = skb_clone(skb, GFP_ATOMIC);
+		if (!skb2) {
+			read_unlock(&mrt_lock);
+			return -ENOMEM;
+		}
+
+		skb2->nh.raw = skb_push(skb2, sizeof(struct iphdr));
+		skb2->nh.iph->ihl = sizeof(struct iphdr)>>2;
+		skb2->nh.iph->saddr = rt->rt_src;
+		skb2->nh.iph->daddr = rt->rt_dst;
+		skb2->nh.iph->version = 0;
+		err = ipmr_cache_unresolved(vif, skb2);
 		read_unlock(&mrt_lock);
 		return err;
 	}
author
#10 lore raw
Quoted 5 lines · On Wed, 26 Jul 2006 01:17:25 +0400 Alexey Kuznetsov <[email protected]>…

Hello!

Quoted 2 lines

Wouldn't it be better to have a consistent interface (skb always freed), and clone the skb if needed for deferred processing?

Quoted 9 lines

I think you mean this.

Note, it is real skb_clone(), not alloc_skb(). Equeued skb contains the whole half-prepared netlink message plus room for the rest. It could be also skb_copy(), if we want to be puristic about mangling cloned data, but original copy is really not going to be used.

Alexey

That is what I was thinking of. Doesn't matter copy or clone, I prefer clone.

maintainer Applied
#11 lore raw
In reply to Alexey Kuznetsov

From: Alexey Kuznetsov <[email protected]>
Date: Wed, 26 Jul 2006 01:17:25 +0400

Quoted 11 lines

Hello!

Quoted 2 lines

Wouldn't it be better to have a consistent interface (skb always freed), and clone the skb if needed for deferred processing?

I think you mean this.

Note, it is real skb_clone(), not alloc_skb(). Equeued skb contains the whole half-prepared netlink message plus room for the rest. It could be also skb_copy(), if we want to be puristic about mangling cloned data, but original copy is really not going to be used.

Applied, thanks Alexey.

#12 lore raw
In reply to Alexey Kuznetsov
Quoted 7 lines · Alexey Kuznetsov

I think you mean this.

Note, it is real skb_clone(), not alloc_skb(). Equeued skb contains the whole half-prepared netlink message plus room for the rest. It could be also skb_copy(), if we want to be puristic about mangling cloned data, but original copy is really not going to be used.

I like this. However, since the cloned skb is either discarded in case of error, or queued in which case the caller discards its reference right away, wouldn't it be simpler to just do this?

Cheers,

Signature · 5 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
net/ipv4/ipmr.c +1 -0
index ba33f86..0a2af08 100644
@@ -1593,6 +1593,7 @@ int ipmr_get_route(struct sk_buff *skb, 
 			read_unlock(&mrt_lock);
 			return -ENODEV;
 		}
+		skb_get(skb);
 		skb->nh.raw = skb_push(skb, sizeof(struct iphdr));
 		skb->nh.iph->ihl = sizeof(struct iphdr)>>2;
 		skb->nh.iph->saddr = rt->rt_src;
#13 lore raw

HellO!

Quoted 3 lines

I like this. However, since the cloned skb is either discarded in case of error, or queued in which case the caller discards its reference right away, wouldn't it be simpler to just do this?

Well, if we wanted just to cheat those checking tools, it is nice. But if we want clarity, it does not look so sweet.

I _loved_ the tricks with skb refcnt a lot (look into netlink.c :-)) until the day, when pskb_expand_head() and Co appeared. It is so much more handy and so hates refcnt.

I would seriously review all the uses of refcnt and leave it internal to skbuff.c and use it externally only in a few places, where it is obviously safe (MSG_PEEK, mostly)

Alexey