* Re: [PATCH] Bugs in xfrm
@ 2004-01-10 18:33 Krishna Kumar
2004-01-10 20:11 ` David S. Miller
0 siblings, 1 reply; 14+ messages in thread
From: Krishna Kumar @ 2004-01-10 18:33 UTC (permalink / raw)
To: David S. Miller; +Cc: krkumar, netdev
[-- Attachment #1: Type: text/plain, Size: 840 bytes --]
> Explain to me how that won't loop forever if given a non-NULL dst?
Oops, forgot the dst = next in the loop :-)
> Next, this dst_bundle_free() thing is totally not needed as far as I can
> tell. When dst_free() is made, the top-level of the bundle's dst gets
added
> to the garbage collection list, the garbage collection properly walks the
> children to process the whole bundle.
The garbage collector (called via __dst_free) takes dst_garbage_list and
goes through the dst->next pointer. But dst_destroy() seems to destroy
stuff
on the dst->child list (I missed the part that the first dst has a refcnt
of
zero and all others on child have refcnt of 1). So this is not needed.
> Please redo this patch and please test it this time :)
Are the other "bugs" correct :-) Or should I send separate patches this
time ?
thanks,
- KK
[-- Attachment #2: Type: text/html, Size: 1338 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bugs in xfrm
2004-01-10 18:33 [PATCH] Bugs in xfrm Krishna Kumar
@ 2004-01-10 20:11 ` David S. Miller
2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar
0 siblings, 1 reply; 14+ messages in thread
From: David S. Miller @ 2004-01-10 20:11 UTC (permalink / raw)
To: Krishna Kumar; +Cc: krkumar, netdev
On Sat, 10 Jan 2004 10:33:22 -0800
Krishna Kumar <kumarkr@us.ibm.com> wrote:
> Or should I send separate patches this time ?
I think this would be a good idea.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state
2004-01-10 20:11 ` David S. Miller
@ 2004-01-13 21:21 ` Krishna Kumar
2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Krishna Kumar @ 2004-01-13 21:21 UTC (permalink / raw)
To: David S. Miller; +Cc: Krishna Kumar, netdev
> > Or should I send separate patches this time ?
>
> I think this would be a good idea.
ipcomp_tunnel_create doesn't set x->km.state to XFRM_STATE_DEAD. This can lead
to the BUG_TRAP in __xfrm_state_destroy when xfrm_state_put() finds this is
the last reference. This was reported as one of the symptoms of [Bug 1754]
some time back.
thanks,
- KK
diff -ruN linux-2.6.0-rc2-bk6.org/net/ipv4/ipcomp.c linux-2.6.0-rc2-bk6/net/ipv4/ipcomp.c
--- linux-2.6.0-rc2-bk6.org/net/ipv4/ipcomp.c 2004-01-05 13:43:50.000000000 -0800
+++ linux-2.6.0-rc2-bk6/net/ipv4/ipcomp.c 2004-01-09 13:00:22.000000000 -0800
@@ -294,6 +294,7 @@
return t;
error:
+ t->km.state = XFRM_STATE_DEAD;
xfrm_state_put(t);
t = NULL;
goto out;
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/5] Bad dereference of xfrm_state in pf_key
2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar
@ 2004-01-13 21:22 ` Krishna Kumar
2004-01-14 7:21 ` David S. Miller
2004-01-13 21:23 ` [PATCH 3/5] xfrm_lookup bugs Krishna Kumar
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Krishna Kumar @ 2004-01-13 21:22 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev
In pfkey_get(), the xfrm_state is dereferenced after it is dropped,
which could lead to dereferencing freed memory. This can also be done
by dropping the reference before the pfkey_broadcast() and in the IS_ERR
case.
thanks,
- KK
diff -ruN linux-2.6.0-rc2-bk6.org/net/key/af_key.c linux-2.6.0-rc2-bk6/net/key/af_key.c
--- linux-2.6.0-rc2-bk6.org/net/key/af_key.c 2004-01-05 13:45:47.000000000 -0800
+++ linux-2.6.0-rc2-bk6/net/key/af_key.c 2004-01-09 12:41:30.000000000 -0800
@@ -1283,6 +1283,7 @@
static int pfkey_get(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
{
+ __u8 proto;
struct sk_buff *out_skb;
struct sadb_msg *out_hdr;
struct xfrm_state *x;
@@ -1297,6 +1298,7 @@
return -ESRCH;
out_skb = pfkey_xfrm_state2msg(x, 1, 3);
+ proto = x->id.proto;
xfrm_state_put(x);
if (IS_ERR(out_skb))
return PTR_ERR(out_skb);
@@ -1304,7 +1306,7 @@
out_hdr = (struct sadb_msg *) out_skb->data;
out_hdr->sadb_msg_version = hdr->sadb_msg_version;
out_hdr->sadb_msg_type = SADB_DUMP;
- out_hdr->sadb_msg_satype = pfkey_proto2satype(x->id.proto);
+ out_hdr->sadb_msg_satype = pfkey_proto2satype(proto);
out_hdr->sadb_msg_errno = 0;
out_hdr->sadb_msg_reserved = 0;
out_hdr->sadb_msg_seq = hdr->sadb_msg_seq;
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/5] xfrm_lookup bugs
2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar
2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar
@ 2004-01-13 21:23 ` Krishna Kumar
2004-01-14 7:23 ` David S. Miller
2004-01-13 21:24 ` [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Krishna Kumar
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Krishna Kumar @ 2004-01-13 21:23 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev
In xfrm_lookup, a couple of bugs :
- the found or allocated xfrm_states are not passed correctly to
xfrm_bundle_create (and to the subsequent frees in case of create
failing) if the first xfrm_tmpl_resolve failed and the second one
succeeded.
- error handling is wrong.
thanks,
- KK
diff -ruN linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c
--- linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c 2004-01-09 12:42:53.000000000 -0800
+++ linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c 2004-01-12 09:55:26.000000000 -0800
@@ -783,25 +783,27 @@
__set_task_state(tsk, TASK_INTERRUPTIBLE);
add_wait_queue(&km_waitq, &wait);
- err = xfrm_tmpl_resolve(policy, fl, xfrm, family);
- if (err == -EAGAIN)
+ nx = xfrm_tmpl_resolve(policy, fl, xfrm, family);
+ if (nx == -EAGAIN)
schedule();
__set_task_state(tsk, TASK_RUNNING);
remove_wait_queue(&km_waitq, &wait);
- if (err == -EAGAIN && signal_pending(current)) {
+ if (nx == -EAGAIN && signal_pending(current)) {
err = -ERESTART;
goto error;
}
- if (err == -EAGAIN ||
+ if (nx == -EAGAIN ||
genid != atomic_read(&flow_cache_genid)) {
xfrm_pol_put(policy);
goto restart;
}
+ err = nx;
}
- if (err)
+ if (err < 0)
goto error;
- } else if (nx == 0) {
+ }
+ if (nx == 0) {
/* Flow passes not transformed. */
xfrm_pol_put(policy);
return 0;
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/5] xfrm_state_construct doesn't set tunnel state
2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar
2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar
2004-01-13 21:23 ` [PATCH 3/5] xfrm_lookup bugs Krishna Kumar
@ 2004-01-13 21:24 ` Krishna Kumar
2004-01-14 7:25 ` David S. Miller
2004-01-13 21:26 ` [PATCH 5/5] schedule() bug in xfrm_lookup() Krishna Kumar
2004-01-14 7:20 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state David S. Miller
4 siblings, 1 reply; 14+ messages in thread
From: Krishna Kumar @ 2004-01-13 21:24 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, KK
xfrm_state_construct doesn't set x->km.state to XFRM_STATE_DEAD. This can lead
to the BUG_TRAP in __xfrm_state_destroy when xfrm_state_put() finds this is
the last reference. This was reported as one of the symptoms of [Bug 1754]
some time back.
thanks,
- KK
diff -ruN linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_user.c linux-2.6.0-rc2-bk6/net/xfrm/xfrm_user.c
--- linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_user.c 2004-01-09 12:57:42.000000000 -0800
+++ linux-2.6.0-rc2-bk6/net/xfrm/xfrm_user.c 2004-01-09 12:59:00.000000000 -0800
@@ -241,6 +241,7 @@
return x;
error:
+ x->km.state = XFRM_STATE_DEAD;
xfrm_state_put(x);
error_no_put:
*errp = err;
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/5] schedule() bug in xfrm_lookup()
2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar
` (2 preceding siblings ...)
2004-01-13 21:24 ` [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Krishna Kumar
@ 2004-01-13 21:26 ` Krishna Kumar
2004-01-14 7:27 ` David S. Miller
2004-01-14 7:20 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state David S. Miller
4 siblings, 1 reply; 14+ messages in thread
From: Krishna Kumar @ 2004-01-13 21:26 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, KK
When xfrm_tmpl_resolve fails, schedule() before retrying. Also, cleaned
up the set_task_* routines to use set_current_*, etc.
This needs to be applied after [PATCH 3/5].
thanks,
- KK
diff -ruN linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c
--- linux-2.6.0-rc2-bk6.org/net/xfrm/xfrm_policy.c 2004-01-09 12:42:53.000000000 -0800
+++ linux-2.6.0-rc2-bk6/net/xfrm/xfrm_policy.c 2004-01-12 09:55:26.000000000 -0800
@@ -775,20 +775,17 @@
if (unlikely(nx<0)) {
err = nx;
- if (err == -EAGAIN) {
- struct task_struct *tsk = current;
- DECLARE_WAITQUEUE(wait, tsk);
- if (!flags)
- goto error;
+ if (err == -EAGAIN && !flags) {
+ DECLARE_WAITQUEUE(wait, current);
- __set_task_state(tsk, TASK_INTERRUPTIBLE);
add_wait_queue(&km_waitq, &wait);
- nx = xfrm_tmpl_resolve(policy, fl, xfrm, family);
- if (nx == -EAGAIN)
- schedule();
- __set_task_state(tsk, TASK_RUNNING);
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule();
+ set_current_state(TASK_RUNNING);
remove_wait_queue(&km_waitq, &wait);
+ nx = xfrm_tmpl_resolve(policy, fl, xfrm, family);
+
if (nx == -EAGAIN && signal_pending(current)) {
err = -ERESTART;
goto error;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state
2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar
` (3 preceding siblings ...)
2004-01-13 21:26 ` [PATCH 5/5] schedule() bug in xfrm_lookup() Krishna Kumar
@ 2004-01-14 7:20 ` David S. Miller
2004-03-09 21:36 ` [PATCH] Trivial kmalloc failure checks Krishna Kumar
4 siblings, 1 reply; 14+ messages in thread
From: David S. Miller @ 2004-01-14 7:20 UTC (permalink / raw)
To: Krishna Kumar; +Cc: krkumar, netdev
On Tue, 13 Jan 2004 13:21:19 -0800 (PST)
Krishna Kumar <krkumar@us.ibm.com> wrote:
> ipcomp_tunnel_create doesn't set x->km.state to XFRM_STATE_DEAD. This can lead
> to the BUG_TRAP in __xfrm_state_destroy when xfrm_state_put() finds this is
> the last reference. This was reported as one of the symptoms of [Bug 1754]
> some time back.
Patch applied, thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] Bad dereference of xfrm_state in pf_key
2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar
@ 2004-01-14 7:21 ` David S. Miller
0 siblings, 0 replies; 14+ messages in thread
From: David S. Miller @ 2004-01-14 7:21 UTC (permalink / raw)
To: Krishna Kumar; +Cc: netdev
On Tue, 13 Jan 2004 13:22:36 -0800 (PST)
Krishna Kumar <krkumar@us.ibm.com> wrote:
> In pfkey_get(), the xfrm_state is dereferenced after it is dropped,
> which could lead to dereferencing freed memory. This can also be done
> by dropping the reference before the pfkey_broadcast() and in the IS_ERR
> case.
Obviously correct, patch applied thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] xfrm_lookup bugs
2004-01-13 21:23 ` [PATCH 3/5] xfrm_lookup bugs Krishna Kumar
@ 2004-01-14 7:23 ` David S. Miller
0 siblings, 0 replies; 14+ messages in thread
From: David S. Miller @ 2004-01-14 7:23 UTC (permalink / raw)
To: Krishna Kumar; +Cc: netdev
On Tue, 13 Jan 2004 13:23:35 -0800 (PST)
Krishna Kumar <krkumar@us.ibm.com> wrote:
> In xfrm_lookup, a couple of bugs :
> - the found or allocated xfrm_states are not passed correctly to
> xfrm_bundle_create (and to the subsequent frees in case of create
> failing) if the first xfrm_tmpl_resolve failed and the second one
> succeeded.
> - error handling is wrong.
Looks fine, patch applied thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/5] xfrm_state_construct doesn't set tunnel state
2004-01-13 21:24 ` [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Krishna Kumar
@ 2004-01-14 7:25 ` David S. Miller
0 siblings, 0 replies; 14+ messages in thread
From: David S. Miller @ 2004-01-14 7:25 UTC (permalink / raw)
To: Krishna Kumar; +Cc: netdev, krkumar
On Tue, 13 Jan 2004 13:24:23 -0800 (PST)
Krishna Kumar <krkumar@us.ibm.com> wrote:
> xfrm_state_construct doesn't set x->km.state to XFRM_STATE_DEAD. This can lead
> to the BUG_TRAP in __xfrm_state_destroy when xfrm_state_put() finds this is
> the last reference. This was reported as one of the symptoms of [Bug 1754]
> some time back.
Smells just like the nearly identical case in ipcomp :-)
Patch applied, thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] schedule() bug in xfrm_lookup()
2004-01-13 21:26 ` [PATCH 5/5] schedule() bug in xfrm_lookup() Krishna Kumar
@ 2004-01-14 7:27 ` David S. Miller
0 siblings, 0 replies; 14+ messages in thread
From: David S. Miller @ 2004-01-14 7:27 UTC (permalink / raw)
To: Krishna Kumar; +Cc: netdev, krkumar
On Tue, 13 Jan 2004 13:26:21 -0800 (PST)
Krishna Kumar <krkumar@us.ibm.com> wrote:
> When xfrm_tmpl_resolve fails, schedule() before retrying. Also, cleaned
> up the set_task_* routines to use set_current_*, etc.
Applied, thanks Krishna.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] Trivial kmalloc failure checks
2004-01-14 7:20 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state David S. Miller
@ 2004-03-09 21:36 ` Krishna Kumar
2004-03-09 23:24 ` David S. Miller
0 siblings, 1 reply; 14+ messages in thread
From: Krishna Kumar @ 2004-03-09 21:36 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, krkumar
Hi Dave,
A couple of checks for memory allocation failures.
Thanks,
- KK
diff -ruN linux-2.6.4-rc2-bk5.org/net/ipv4/esp4.c linux-2.6.4-rc2-bk5/net/ipv4/esp4.c
--- linux-2.6.4-rc2-bk5.org/net/ipv4/esp4.c 2004-03-09 13:11:15.000000000 -0800
+++ linux-2.6.4-rc2-bk5/net/ipv4/esp4.c 2004-03-09 13:29:44.000000000 -0800
@@ -518,6 +518,8 @@
esp->conf.padlen = 0;
if (esp->conf.ivlen) {
esp->conf.ivec = kmalloc(esp->conf.ivlen, GFP_KERNEL);
+ if (unlikely(esp->conf.ivec == NULL))
+ goto error;
get_random_bytes(esp->conf.ivec, esp->conf.ivlen);
}
crypto_cipher_setkey(esp->conf.tfm, esp->conf.key, esp->conf.key_len);
diff -ruN linux-2.6.4-rc2-bk5.org/net/ipv4/ip_output.c linux-2.6.4-rc2-bk5/net/ipv4/ip_output.c
--- linux-2.6.4-rc2-bk5.org/net/ipv4/ip_output.c 2004-03-09 13:18:50.000000000 -0800
+++ linux-2.6.4-rc2-bk5/net/ipv4/ip_output.c 2004-03-09 13:20:10.000000000 -0800
@@ -761,8 +761,11 @@
*/
opt = ipc->opt;
if (opt) {
- if (inet->cork.opt == NULL)
+ if (inet->cork.opt == NULL) {
inet->cork.opt = kmalloc(sizeof(struct ip_options) + 40, sk->sk_allocation);
+ if (unlikely(inet->cork.opt == NULL))
+ return -ENOBUFS;
+ }
memcpy(inet->cork.opt, opt, sizeof(struct ip_options)+opt->optlen);
inet->cork.flags |= IPCORK_OPT;
inet->cork.addr = ipc->addr;
diff -ruN linux-2.6.4-rc2-bk5.org/net/ipv6/esp6.c linux-2.6.4-rc2-bk5/net/ipv6/esp6.c
--- linux-2.6.4-rc2-bk5.org/net/ipv6/esp6.c 2004-03-09 13:10:42.000000000 -0800
+++ linux-2.6.4-rc2-bk5/net/ipv6/esp6.c 2004-03-09 13:30:03.000000000 -0800
@@ -422,6 +422,8 @@
esp->conf.padlen = 0;
if (esp->conf.ivlen) {
esp->conf.ivec = kmalloc(esp->conf.ivlen, GFP_KERNEL);
+ if (unlikely(esp->conf.ivec == NULL))
+ goto error;
get_random_bytes(esp->conf.ivec, esp->conf.ivlen);
}
crypto_cipher_setkey(esp->conf.tfm, esp->conf.key, esp->conf.key_len);
diff -ruN linux-2.6.4-rc2-bk5.org/net/ipv6/ip6_output.c linux-2.6.4-rc2-bk5/net/ipv6/ip6_output.c
--- linux-2.6.4-rc2-bk5.org/net/ipv6/ip6_output.c 2004-03-09 13:10:42.000000000 -0800
+++ linux-2.6.4-rc2-bk5/net/ipv6/ip6_output.c 2004-03-09 13:20:29.000000000 -0800
@@ -816,9 +816,12 @@
* setup for corking
*/
if (opt) {
- if (np->cork.opt == NULL)
+ if (np->cork.opt == NULL) {
np->cork.opt = kmalloc(opt->tot_len,
sk->sk_allocation);
+ if (unlikely(np->cork.opt == NULL))
+ return -ENOBUFS;
+ }
memcpy(np->cork.opt, opt, opt->tot_len);
inet->cork.flags |= IPCORK_OPT;
/* need source address above miyazawa*/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Trivial kmalloc failure checks
2004-03-09 21:36 ` [PATCH] Trivial kmalloc failure checks Krishna Kumar
@ 2004-03-09 23:24 ` David S. Miller
0 siblings, 0 replies; 14+ messages in thread
From: David S. Miller @ 2004-03-09 23:24 UTC (permalink / raw)
To: Krishna Kumar; +Cc: netdev, krkumar
On Tue, 9 Mar 2004 13:36:14 -0800 (PST)
Krishna Kumar <krkumar@us.ibm.com> wrote:
> A couple of checks for memory allocation failures.
Looks great, applied.
Thanks Krishna.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2004-03-09 23:24 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-01-10 18:33 [PATCH] Bugs in xfrm Krishna Kumar
2004-01-10 20:11 ` David S. Miller
2004-01-13 21:21 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state Krishna Kumar
2004-01-13 21:22 ` [PATCH 2/5] Bad dereference of xfrm_state in pf_key Krishna Kumar
2004-01-14 7:21 ` David S. Miller
2004-01-13 21:23 ` [PATCH 3/5] xfrm_lookup bugs Krishna Kumar
2004-01-14 7:23 ` David S. Miller
2004-01-13 21:24 ` [PATCH 4/5] xfrm_state_construct doesn't set tunnel state Krishna Kumar
2004-01-14 7:25 ` David S. Miller
2004-01-13 21:26 ` [PATCH 5/5] schedule() bug in xfrm_lookup() Krishna Kumar
2004-01-14 7:27 ` David S. Miller
2004-01-14 7:20 ` [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state David S. Miller
2004-03-09 21:36 ` [PATCH] Trivial kmalloc failure checks Krishna Kumar
2004-03-09 23:24 ` David S. Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.