Re: [PATCH] ip multicast route bug fix
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ip multicast route bug fix
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
signature.asc
Description: This is a digitally signed message part
Re: [PATCH] ip multicast route bug fix
Stephen Hemminger [EMAIL PROTECTED] 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 ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ip multicast route bug fix
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.qlen3) {
- 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;
-
To unsubscribe
[PATCH] 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(-)
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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ip multicast route bug fix
On Tue, 25 Jul 2006 20:20:01 +0400 Alexey Kuznetsov [EMAIL PROTECTED] 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? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ip multicast route bug fix
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ip multicast route bug fix
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ip multicast route bug fix
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;
}
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ip multicast route bug fix
On Wed, 26 Jul 2006 01:17:25 +0400 Alexey Kuznetsov [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ip multicast route bug fix
Alexey Kuznetsov [EMAIL PROTECTED] 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 ~{PmVHI~} [EMAIL PROTECTED]
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;
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] 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(-)
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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ip multicast route bug fix
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 [EMAIL PROTECTED] Acked-by: James Morris [EMAIL PROTECTED] -- James Morris [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
