Re: [PATCH] Bugs in xfrm

Re: [PATCH] Bugs in xfrm

[Krishna Kumar]

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

Re: [PATCH] Bugs in xfrm

[David S. Miller]

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.

[PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state

[Krishna Kumar]

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

[PATCH 2/5] Bad dereference of xfrm_state in pf_key

[Krishna Kumar]

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;

[PATCH 3/5] xfrm_lookup bugs

[Krishna Kumar]

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;

[PATCH 4/5] xfrm_state_construct doesn't set tunnel state

[Krishna Kumar]

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;

[PATCH 5/5] schedule() bug in xfrm_lookup()

[Krishna Kumar]

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;

Re: [PATCH 1/5] ipcomp_tunnel_create doesn't set tunnel state

[David S. Miller]

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.

Re: [PATCH 2/5] Bad dereference of xfrm_state in pf_key

[David S. Miller]

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.

Re: [PATCH 3/5] xfrm_lookup bugs

[David S. Miller]

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.

Re: [PATCH 4/5] xfrm_state_construct doesn't set tunnel state

[David S. Miller]

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.

Re: [PATCH 5/5] schedule() bug in xfrm_lookup()

[David S. Miller]

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.

[PATCH] Trivial kmalloc failure checks

[Krishna Kumar]

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*/

Re: [PATCH] Trivial kmalloc failure checks

[David S. Miller]

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.