[CRYPTO] Fix stack overrun in crypt()

[CRYPTO] Fix stack overrun in crypt()

[Herbert Xu]

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

Hi:

The stack allocation in crypt() is bogus as whether tmp_src/tmp_dst
is used is determined by factors unrelated to nbytes and
src->length/dst->length.

Since the condition for whether tmp_src/tmp_dst are used is very
complex, let's allocate them always instead of guessing.

This fixes a number of weird crashes including those AES crashes
that people have been seeing with the 2.4 backport + ipt_conntrack.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

PS I think someone should double-check the logic in the scatterwalk
stuff, especially the whichbuf bits.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 439 bytes --]

===== crypto/cipher.c 1.18 vs edited =====
--- 1.18/crypto/cipher.c	2004-05-27 06:25:36 +10:00
+++ edited/crypto/cipher.c	2004-07-15 21:40:53 +10:00
@@ -52,8 +52,8 @@
 {
 	struct scatter_walk walk_in, walk_out;
 	const unsigned int bsize = crypto_tfm_alg_blocksize(tfm);
-	u8 tmp_src[nbytes > src->length ? bsize : 0];
-	u8 tmp_dst[nbytes > dst->length ? bsize : 0];
+	u8 tmp_src[bsize];
+	u8 tmp_dst[bsize];
 
 	if (!nbytes)
 		return 0;

Re: [CRYPTO] Fix stack overrun in crypt()

[James Morris]

On Thu, 15 Jul 2004, Herbert Xu wrote:

> Hi:
> 
> The stack allocation in crypt() is bogus as whether tmp_src/tmp_dst
> is used is determined by factors unrelated to nbytes and
> src->length/dst->length.
> 
> Since the condition for whether tmp_src/tmp_dst are used is very
> complex, let's allocate them always instead of guessing.
> 
> This fixes a number of weird crashes including those AES crashes
> that people have been seeing with the 2.4 backport + ipt_conntrack.

Ok, thanks, looks good.

> PS I think someone should double-check the logic in the scatterwalk
> stuff, especially the whichbuf bits.

Adam Richter rewrote that code, and I have walked through it before (I 
guess Dave did too).  Any more code reviewers welcome.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [CRYPTO] Fix stack overrun in crypt()

[Herbert Xu]

On Fri, Jul 16, 2004 at 11:27:36AM -0400, James Morris wrote:
>
> > This fixes a number of weird crashes including those AES crashes
> > that people have been seeing with the 2.4 backport + ipt_conntrack.
> 
> Ok, thanks, looks good.

Thanks for reviewing it.

Unfortunately it looks like we still have a problem. gcc 3.3.4 appears
to be generating incorrect output on i386 with the dynamic stack
allocation used in crypt() and the functions around it.

In particular, it can give you 8 bytes when you ask for 16 bytes.
See my report at http://bugs.debian.org/259887 for details.

Fortunately, it seems that overwriting 8 bytes beyond the end of
the array in crypt() is not fatal.  After all, that's why people
only saw crashes with AES and not 3DES.

But this is still a potential source of problem, especially given
algorithms with bigger block sizes.

I think we should stop people from building the kernel with gcc 3.3.*
until this problem is addressed.  What do you guys think?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [CRYPTO] Fix stack overrun in crypt()

[Herbert Xu]

On Sat, Jul 17, 2004 at 05:43:19PM +1000, herbert wrote:
> 
> Unfortunately it looks like we still have a problem. gcc 3.3.4 appears
> to be generating incorrect output on i386 with the dynamic stack
> allocation used in crypt() and the functions around it.
> 
> In particular, it can give you 8 bytes when you ask for 16 bytes.
> See my report at http://bugs.debian.org/259887 for details.

I got it wrong.  gcc is simply allocating some (12 bytes) of the
space unconditionally.

Sorry for the noise.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [CRYPTO] Fix stack overrun in crypt()

[David S. Miller]

On Thu, 15 Jul 2004 21:48:40 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The stack allocation in crypt() is bogus as whether tmp_src/tmp_dst
> is used is determined by factors unrelated to nbytes and
> src->length/dst->length.
> 
> Since the condition for whether tmp_src/tmp_dst are used is very
> complex, let's allocate them always instead of guessing.
> 
> This fixes a number of weird crashes including those AES crashes
> that people have been seeing with the 2.4 backport + ipt_conntrack.

Applied, thanks Herbert.

> PS I think someone should double-check the logic in the scatterwalk
> stuff, especially the whichbuf bits.

I've looked at this before, when it went in, but I'll double-
check it now.

Re: [CRYPTO] Fix stack overrun in crypt()

[David S. Miller]

On Thu, 15 Jul 2004 21:48:40 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> PS I think someone should double-check the logic in the scatterwalk
> stuff, especially the whichbuf bits.

The goal of scatterwalk_whichbuf() is to use the temporary buffer
if we are walking over a page boundary.

We can use walk->data, and thus directly the page involved, if
we do not cross such a boundary.  The test is that all of the
following conditions pass:

	1) nbytes is <= walk->len_this_page

	   When scatterwalk_start() is invoked, walk->len_this_page
	   is set to the minimum of the remaining scatterlist segment
	   length and the remaining bytes in that page itself.

	2) walk->data + nbytes does not straddle a PAGE_CACHE_SIZE
	   boundary

This looks all fine to me.