Bug somewhere in crypto or ipsec stuff

Bug somewhere in crypto or ipsec stuff

[Paul Mackerras]

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.

Re: Bug somewhere in crypto or ipsec stuff

[YOSHIFUJI Hideaki / 吉藤英明]

In article <16288.30574.745348.194005@cargo.ozlabs.ibm.com> (at Thu, 30 Oct 2003 13:29:02 +1100), Paul Mackerras <paulus@samba.org> 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

Re: Bug somewhere in crypto or ipsec stuff

[James Morris]

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
<jmorris@redhat.com>

Re: Bug somewhere in crypto or ipsec stuff

[YOSHIFUJI Hideaki / 吉藤英明]

In article <Xine.LNX.4.44.0310292221320.23405-100000@thoron.boston.redhat.com> (at Wed, 29 Oct 2003 22:22:50 -0500 (EST)), James Morris <jmorris@redhat.com> says:

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

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

Re: Bug somewhere in crypto or ipsec stuff

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

On Thu, 30 Oct 2003 12:41:24 +0900, YOSHIFUJI Hideaki / =?iso-2022-jp?B?GyRCNUhGIzFRTEAbKEI=?= said:

> 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.)

This could be embarasssing if somebody happened to have the rot13 debugging
algorithm loaded.....

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Bug somewhere in crypto or ipsec stuff

[James Morris]

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

> In article <Xine.LNX.4.44.0310292221320.23405-100000@thoron.boston.redhat.com> (at Wed, 29 Oct 2003 22:22:50 -0500 (EST)), James Morris <jmorris@redhat.com> says:
> 
> > 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.
> 
> 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
<jmorris@redhat.com>

Re: Bug somewhere in crypto or ipsec stuff

[YOSHIFUJI Hideaki / 吉藤英明]

In article <Xine.LNX.4.44.0310292344530.23580-100000@thoron.boston.redhat.com> (at Wed, 29 Oct 2003 23:47:05 -0500 (EST)), James Morris <jmorris@redhat.com> says:

> > Do you mean that we need to fix the caller?
> 
> Yes.

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