Bug somewhere in crypto or ipsec stuff

I get this oops in strcmp, called from crypto_alg_lookup, when I run
the "spi" command from a freeswan snapshot from 13 October this year.
The kernel is 2.6.0-test9.

Oops: kernel access of bad area, sig: 11 [#1]
NIP: C001323C LR: C00CEE6C SP: CB8D1C60 REGS: cb8d1bb0 TRAP: 0301    Not tainted
MSR: 00009032 EE: 1 PR: 0 FP: 0 ME: 1 IR/DR: 11
DAR: 00000000, DSISR: 40000000
TASK = ca0c2320[934] 'spi' Last syscall: 4 
GPR00: C02D5204 CB8D1C60 CA0C2320 00000063 FFFFFFFF C02D5284 CB472A38 00000003 
GPR08: C03D0C84 C033BCBC 00000000 C00CF03C C03D0C84 
Call trace:
 [c00cf058] crypto_alloc_tfm+0x1c/0x104
 [cd97fb34] ipcomp_init_state+0x90/0x118 [ipcomp]
 [c0233a2c] pfkey_msg2xfrm_state+0x598/0x740
 [c0233ed4] pfkey_add+0x2c/0x148
 [c0235bb4] pfkey_process+0xb8/0xc0
 [c0236ba0] pfkey_sendmsg+0x124/0x204
 [c01c2018] sock_aio_write+0xe8/0x104
 [c0054e6c] do_sync_write+0x74/0xb8
 [c0054fc0] vfs_write+0x110/0x128
 [c005508c] sys_write+0x40/0x74
 [c0007a9c] ret_from_syscall+0x0/0x44

The problem is basically that crypto_alg_lookup gets called with NULL
for the `name' parameter.

The command that provokes the oops is:

spi --af inet --said tun.1234@10.61.2.68 --ip4 --src 10.61.2.90 \
    --dst 10.61.2.68

I was trying this because the freeswan web pages claim that they have
some preliminary support for 2.6 in their user command set.  I was
only half expecting the spi command to work, but it shouldn't have
been able to cause an oops.

Paul.

In article [off-list ref] (at Thu, 30 Oct 2003 13:29:02 +1100), Paul Mackerras [off-list ref] says:

I get this oops in strcmp, called from crypto_alg_lookup, when I run
the "spi" command from a freeswan snapshot from 13 October this year.
The kernel is 2.6.0-test9.

Oops: kernel access of bad area, sig: 11 [#1]

:

Call trace:
 [c00cf058] crypto_alloc_tfm+0x1c/0x104
 [cd97fb34] ipcomp_init_state+0x90/0x118 [ipcomp]

:

The problem is basically that crypto_alg_lookup gets called with NULL
for the `name' parameter.

I would just disallow name == NULL,
well, what algorithm do you expect?

===== crypto/api.c 1.30 vs edited =====

--- 1.30/crypto/api.c	Sat Mar 29 20:16:58 2003
+++ edited/crypto/api.c	Thu Oct 30 12:07:43 2003

@@ -36,6 +36,9 @@
 struct crypto_alg *crypto_alg_lookup(const char *name)
 {
 	struct crypto_alg *q, *alg = NULL;
+
+	if (!name)
+		return NULL;
 	
 	down_read(&crypto_alg_sem);
 	

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

On Thu, 30 Oct 2003, YOSHIFUJI Hideaki / [iso-2022-jp] $B5HF#1QL@(B wrote:

I would just disallow name == NULL,
well, what algorithm do you expect?

Good question.  It seems to me to be a bug in the calling code if it is 
trying to look up nothing -- I'd rather not paper that over.


- James
-- 
James Morris
[off-list ref]

In article [off-list ref] (at Wed, 29 Oct 2003 22:22:50 -0500 (EST)), James Morris [off-list ref] says:

On Thu, 30 Oct 2003, YOSHIFUJI Hideaki / [iso-2022-jp] $B5HF#1QL@(B wrote:

quoted

I would just disallow name == NULL,
well, what algorithm do you expect?

Good question.  It seems to me to be a bug in the calling code if it is 
trying to look up nothing -- I'd rather not paper that over.

Do you mean that we need to fix the caller?

Well, people may want to get just any algorithm.
In such case,
 - crypto allows name == NULL, and return any algorithm
   (for example, an algorithm that we see first.)
 - caller may filter name == NULL case if it is ambiguous in their context.

--yoshfuji

On Thu, 30 Oct 2003, YOSHIFUJI Hideaki / [iso-2022-jp] $B5HF#1QL@(B wrote:

In article [off-list ref] (at Wed, 29 Oct 2003 22:22:50 -0500 (EST)), James Morris [off-list ref] says:

quoted

On Thu, 30 Oct 2003, YOSHIFUJI Hideaki / [iso-2022-jp] $B5HF#1QL@(B wrote:

quoted

I would just disallow name == NULL,
well, what algorithm do you expect?

Good question.  It seems to me to be a bug in the calling code if it is 
trying to look up nothing -- I'd rather not paper that over.

Do you mean that we need to fix the caller?

Yes.

Well, people may want to get just any algorithm.
In such case,
 - crypto allows name == NULL, and return any algorithm
   (for example, an algorithm that we see first.)
 - caller may filter name == NULL case if it is ambiguous in their context.

I think that could be dangerous, including if calling with null is a 
bug, and they get an inappropriate algorithm.  An incorrect algorithm type 
could also be returned (e.g. digest instead of a cipher).

- James
-- 
James Morris
[off-list ref]

In article [off-list ref] (at Wed, 29 Oct 2003 23:47:05 -0500 (EST)), James Morris [off-list ref] says:

quoted

Do you mean that we need to fix the caller?

Yes.

quoted

 - crypto allows name == NULL, and return any algorithm
   (for example, an algorithm that we see first.)
 - caller may filter name == NULL case if it is ambiguous in their context.

I think that could be dangerous, including if calling with null is a 
bug, and they get an inappropriate algorithm.  An incorrect algorithm type 
could also be returned (e.g. digest instead of a cipher).

okay. how about this?

===== crypto/api.c 1.30 vs edited =====

--- 1.30/crypto/api.c	Sat Mar 29 20:16:58 2003
+++ edited/crypto/api.c	Thu Oct 30 14:21:53 2003

@@ -36,6 +36,9 @@
 struct crypto_alg *crypto_alg_lookup(const char *name)
 {
 	struct crypto_alg *q, *alg = NULL;
+
+	if (!name)
+		return NULL;
 	
 	down_read(&crypto_alg_sem);
 	

===== net/ipv4/ipcomp.c 1.16 vs edited =====

--- 1.16/net/ipv4/ipcomp.c	Mon Aug 18 20:14:38 2003
+++ edited/net/ipv4/ipcomp.c	Thu Oct 30 14:18:49 2003

@@ -360,7 +360,12 @@
 	ipcd->scratch = kmalloc(IPCOMP_SCRATCH_SIZE, GFP_KERNEL);
 	if (!ipcd->scratch)
 		goto error;
-	
+
+	if (!x->calg->alg_name) {
+		err = -EINVAL;
+		goto error;
+	}
+
 	ipcd->tfm = crypto_alloc_tfm(x->calg->alg_name, 0);
 	if (!ipcd->tfm)
 		goto error;

===== net/ipv6/ipcomp6.c 1.7 vs edited =====

--- 1.7/net/ipv6/ipcomp6.c	Mon Aug 18 20:14:38 2003
+++ edited/net/ipv6/ipcomp6.c	Thu Oct 30 14:18:49 2003

@@ -293,6 +293,11 @@
 	if (!ipcd->scratch)
 		goto error;
 
+	if (!x->calg->alg_name) {
+		err = -EINVAL;
+		goto error;
+	}
+
 	ipcd->tfm = crypto_alloc_tfm(x->calg->alg_name, 0);
 	if (!ipcd->tfm)
 		goto error;