ip multicast route bug fix
Subsystems (3)
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.
--- net/ipv4/ipmr.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
net/ipv4/ipmr.c +14 -6
@@ -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
Quoted 6 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.
Signed-off-by Stephen Hemminger <[email protected]>
Signature · 2 lines
James Morris <[email protected]>
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
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.txtPATCH 1/1 ip multicast route bug fix (rev2)
IP multicast route code was reusing an skb which causes use after free and double free.
--- net/ipv4/ipmr.c | 22 ++++++++++++++++------ 1 files changed, 16 insertions(+), 6 deletions(-)
net/ipv4/ipmr.c +16 -6
@@ -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
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
@@ -640,8 +640,6 @@ ipmr_cache_unresolved(vifi_t vifi, strucif (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, strucspin_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
@@ -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;
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 ininet_rtm_getroute. But after this fix, which introduced new bug (gotoout_freein the enclosed patch), the bug showed on surface.
Quoted 6 lines
Please, review this.
-
ipmr_cache_unresolved()does not free skb. Caller does.
- gotoout_freeinroute.cin the case, when skb is enqueued is returned to goto out.
- some cosmetic cleanup inrt_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?
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
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
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
@@ -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; }
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(), notalloc_skb(). Equeued skb contains the whole half-prepared netlink message plus room for the rest. It could be alsoskb_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.
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(), notalloc_skb(). Equeued skb contains the whole half-prepared netlink message plus room for the rest. It could be alsoskb_copy(), if we want to be puristic about mangling cloned data, but original copy is really not going to be used.
Applied, thanks Alexey.
Quoted 7 lines · Alexey Kuznetsov
I think you mean this.
Note, it is real
skb_clone(), notalloc_skb(). Equeued skb contains the whole half-prepared netlink message plus room for the rest. It could be alsoskb_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
@@ -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;
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