* [CRYPTO] Fix stack overrun in crypt()
@ 2004-07-15 11:48 Herbert Xu
2004-07-16 15:27 ` James Morris
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Herbert Xu @ 2004-07-15 11:48 UTC (permalink / raw)
To: David S. Miller; +Cc: James Morris, netdev
[-- 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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [CRYPTO] Fix stack overrun in crypt()
2004-07-15 11:48 [CRYPTO] Fix stack overrun in crypt() Herbert Xu
@ 2004-07-16 15:27 ` James Morris
2004-07-17 7:43 ` Herbert Xu
2004-07-21 21:58 ` David S. Miller
2004-07-21 22:02 ` David S. Miller
2 siblings, 1 reply; 6+ messages in thread
From: James Morris @ 2004-07-16 15:27 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [CRYPTO] Fix stack overrun in crypt()
2004-07-16 15:27 ` James Morris
@ 2004-07-17 7:43 ` Herbert Xu
2004-07-17 9:48 ` Herbert Xu
0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2004-07-17 7:43 UTC (permalink / raw)
To: James Morris; +Cc: David S. Miller, netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [CRYPTO] Fix stack overrun in crypt()
2004-07-15 11:48 [CRYPTO] Fix stack overrun in crypt() Herbert Xu
2004-07-16 15:27 ` James Morris
@ 2004-07-21 21:58 ` David S. Miller
2004-07-21 22:02 ` David S. Miller
2 siblings, 0 replies; 6+ messages in thread
From: David S. Miller @ 2004-07-21 21:58 UTC (permalink / raw)
To: Herbert Xu; +Cc: jmorris, netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [CRYPTO] Fix stack overrun in crypt()
2004-07-15 11:48 [CRYPTO] Fix stack overrun in crypt() Herbert Xu
2004-07-16 15:27 ` James Morris
2004-07-21 21:58 ` David S. Miller
@ 2004-07-21 22:02 ` David S. Miller
2 siblings, 0 replies; 6+ messages in thread
From: David S. Miller @ 2004-07-21 22:02 UTC (permalink / raw)
To: Herbert Xu; +Cc: jmorris, netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-07-21 22:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-07-15 11:48 [CRYPTO] Fix stack overrun in crypt() Herbert Xu
2004-07-16 15:27 ` James Morris
2004-07-17 7:43 ` Herbert Xu
2004-07-17 9:48 ` Herbert Xu
2004-07-21 21:58 ` David S. Miller
2004-07-21 22:02 ` David S. Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).