[PATCH] ip multicast route bug fix

[PATCH] ip multicast route bug fix

[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.

Signed-off-by: Stephen Hemminger <shemminger@dxpl.pdx.osdl.net>
---
 net/ipv4/ipmr.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index ba33f86..d336104 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -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;
 	}
-- 
1.4.0

Re: [PATCH] ip multicast route bug fix

[James Morris]

On Wed, 19 Jul 2006, Stephen Hemminger wrote:

> 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 <shemminger@dxpl.pdx.osdl.net>

Acked-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] ip multicast route bug fix

[Martin Josefsson]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

On Wed, 2006-07-19 at 14:57 -0700, Stephen Hemminger wrote:
> 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.

> +		
> +		iskb = alloc_skb(sizeof(struct iphdr), GFP_KERNEL);
> +		if (!iskb) {
> +			read_unlock(&mrt_lock);
> +			return -ENOMEM;
> +		}

GFP_KERNEL allocation under read_lock()?

-- 
/Martin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] ip multicast route bug fix

[Herbert Xu]

Stephen Hemminger <shemminger@osdl.org> wrote:
>
> @@ -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);

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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] ip multicast route bug fix (rev2)

[Stephen Hemminger]

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

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
---
 net/ipv4/ipmr.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index ba33f86..f5d3d96 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -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;
 	}
-- 
1.4.0

Re: [PATCH] ip multicast route bug fix

[Stephen Hemminger]

On Tue, 25 Jul 2006 20:20:01 +0400
Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> wrote:

> Hello!
> 
> > 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.

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?

Re: [PATCH] ip multicast route bug fix

[Alexey Kuznetsov]

Hello!

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

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 9ccacf5..1788c04 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -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;
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 2dc6dbb..62f53e9 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -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;



 

Re: [PATCH] ip multicast route bug fix

[Alexey Kuznetsov]

Hello!

> 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

Re: [PATCH] ip multicast route bug fix

[Alexey Kuznetsov]

Hello!

> 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

Re: [PATCH] ip multicast route bug fix

[Alexey Kuznetsov]

Hello!

> 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


diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 9ccacf5..85893ee 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -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;
 	}

Re: [PATCH] ip multicast route bug fix

[Stephen Hemminger]

On Wed, 26 Jul 2006 01:17:25 +0400
Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> wrote:

> Hello!
> 
> > 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
> 

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

Re: [PATCH] ip multicast route bug fix

[David Miller]

From: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Date: Wed, 26 Jul 2006 01:17:25 +0400

> Hello!
> 
> > 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.

Re: [PATCH] ip multicast route bug fix

[Herbert Xu]

Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> wrote:
> 
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index ba33f86..0a2af08 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -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;

Re: [PATCH] ip multicast route bug fix

[Alexey Kuznetsov]

HellO!

> 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