linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] crypto: testmgr - reduce stack usage in fuzzers
@ 2019-06-17 13:23 Arnd Bergmann
  2019-06-17 14:04 ` Herbert Xu
  2019-06-17 17:20 ` Eric Biggers
  0 siblings, 2 replies; 10+ messages in thread
From: Arnd Bergmann @ 2019-06-17 13:23 UTC (permalink / raw)
  To: Herbert Xu, David S. Miller
  Cc: Arnd Bergmann, Eric Biggers, Ard Biesheuvel, Vitaly Chikunov,
	Gilad Ben-Yossef, linux-crypto, linux-kernel

On arm32, we get warnings about high stack usage in some of the functions:

crypto/testmgr.c:2269:12: error: stack frame size of 1032 bytes in function 'alg_test_aead' [-Werror,-Wframe-larger-than=]
static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
           ^
crypto/testmgr.c:1693:12: error: stack frame size of 1312 bytes in function '__alg_test_hash' [-Werror,-Wframe-larger-than=]
static int __alg_test_hash(const struct hash_testvec *vecs,
           ^

On of the larger objects on the stack here is struct testvec_config, so
change that to dynamic allocation.

Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I only compile-tested this, and it's not completely trivial, so please
review carefully.
---
 crypto/testmgr.c | 61 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 6c28055d41ca..7928296cdcb3 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1503,13 +1503,15 @@ static int test_hash_vec(const char *driver, const struct hash_testvec *vec,
  * Generate a hash test vector from the given implementation.
  * Assumes the buffers in 'vec' were already allocated.
  */
-static void generate_random_hash_testvec(struct crypto_shash *tfm,
+static int generate_random_hash_testvec(struct crypto_shash *tfm,
 					 struct hash_testvec *vec,
 					 unsigned int maxkeysize,
 					 unsigned int maxdatasize,
 					 char *name, size_t max_namelen)
 {
-	SHASH_DESC_ON_STACK(desc, tfm);
+	struct shash_desc *desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(tfm), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
 
 	/* Data */
 	vec->psize = generate_random_length(maxdatasize);
@@ -1541,6 +1543,10 @@ static void generate_random_hash_testvec(struct crypto_shash *tfm,
 done:
 	snprintf(name, max_namelen, "\"random: psize=%u ksize=%u\"",
 		 vec->psize, vec->ksize);
+
+	kfree(desc);
+
+	return 0;
 }
 
 /*
@@ -1565,7 +1571,7 @@ static int test_hash_vs_generic_impl(const char *driver,
 	unsigned int i;
 	struct hash_testvec vec = { 0 };
 	char vec_name[64];
-	struct testvec_config cfg;
+	struct testvec_config *cfg;
 	char cfgname[TESTVEC_CONFIG_NAMELEN];
 	int err;
 
@@ -1595,6 +1601,12 @@ static int test_hash_vs_generic_impl(const char *driver,
 		return err;
 	}
 
+	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+	if (!cfg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	/* Check the algorithm properties for consistency. */
 
 	if (digestsize != crypto_shash_digestsize(generic_tfm)) {
@@ -1626,12 +1638,14 @@ static int test_hash_vs_generic_impl(const char *driver,
 	}
 
 	for (i = 0; i < fuzz_iterations * 8; i++) {
-		generate_random_hash_testvec(generic_tfm, &vec,
-					     maxkeysize, maxdatasize,
-					     vec_name, sizeof(vec_name));
-		generate_random_testvec_config(&cfg, cfgname, sizeof(cfgname));
+		err = generate_random_hash_testvec(generic_tfm, &vec,
+						   maxkeysize, maxdatasize,
+						   vec_name, sizeof(vec_name));
+		if (err)
+			goto out;
+		generate_random_testvec_config(cfg, cfgname, sizeof(cfgname));
 
-		err = test_hash_vec_cfg(driver, &vec, vec_name, &cfg,
+		err = test_hash_vec_cfg(driver, &vec, vec_name, cfg,
 					req, desc, tsgl, hashstate);
 		if (err)
 			goto out;
@@ -1639,6 +1653,7 @@ static int test_hash_vs_generic_impl(const char *driver,
 	}
 	err = 0;
 out:
+	kfree(cfg);
 	kfree(vec.key);
 	kfree(vec.plaintext);
 	kfree(vec.digest);
@@ -2135,7 +2150,7 @@ static int test_aead_vs_generic_impl(const char *driver,
 	unsigned int i;
 	struct aead_testvec vec = { 0 };
 	char vec_name[64];
-	struct testvec_config cfg;
+	struct testvec_config *cfg;
 	char cfgname[TESTVEC_CONFIG_NAMELEN];
 	int err;
 
@@ -2165,6 +2180,12 @@ static int test_aead_vs_generic_impl(const char *driver,
 		return err;
 	}
 
+	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+	if (!cfg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	generic_req = aead_request_alloc(generic_tfm, GFP_KERNEL);
 	if (!generic_req) {
 		err = -ENOMEM;
@@ -2219,13 +2240,13 @@ static int test_aead_vs_generic_impl(const char *driver,
 		generate_random_aead_testvec(generic_req, &vec,
 					     maxkeysize, maxdatasize,
 					     vec_name, sizeof(vec_name));
-		generate_random_testvec_config(&cfg, cfgname, sizeof(cfgname));
+		generate_random_testvec_config(cfg, cfgname, sizeof(cfgname));
 
-		err = test_aead_vec_cfg(driver, ENCRYPT, &vec, vec_name, &cfg,
+		err = test_aead_vec_cfg(driver, ENCRYPT, &vec, vec_name, cfg,
 					req, tsgls);
 		if (err)
 			goto out;
-		err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name, &cfg,
+		err = test_aead_vec_cfg(driver, DECRYPT, &vec, vec_name, cfg,
 					req, tsgls);
 		if (err)
 			goto out;
@@ -2233,6 +2254,7 @@ static int test_aead_vs_generic_impl(const char *driver,
 	}
 	err = 0;
 out:
+	kfree(cfg);
 	kfree(vec.key);
 	kfree(vec.iv);
 	kfree(vec.assoc);
@@ -2682,7 +2704,7 @@ static int test_skcipher_vs_generic_impl(const char *driver,
 	unsigned int i;
 	struct cipher_testvec vec = { 0 };
 	char vec_name[64];
-	struct testvec_config cfg;
+	struct testvec_config *cfg;
 	char cfgname[TESTVEC_CONFIG_NAMELEN];
 	int err;
 
@@ -2716,6 +2738,12 @@ static int test_skcipher_vs_generic_impl(const char *driver,
 		return err;
 	}
 
+	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+	if (!cfg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	generic_req = skcipher_request_alloc(generic_tfm, GFP_KERNEL);
 	if (!generic_req) {
 		err = -ENOMEM;
@@ -2763,20 +2791,21 @@ static int test_skcipher_vs_generic_impl(const char *driver,
 	for (i = 0; i < fuzz_iterations * 8; i++) {
 		generate_random_cipher_testvec(generic_req, &vec, maxdatasize,
 					       vec_name, sizeof(vec_name));
-		generate_random_testvec_config(&cfg, cfgname, sizeof(cfgname));
+		generate_random_testvec_config(cfg, cfgname, sizeof(cfgname));
 
 		err = test_skcipher_vec_cfg(driver, ENCRYPT, &vec, vec_name,
-					    &cfg, req, tsgls);
+					    cfg, req, tsgls);
 		if (err)
 			goto out;
 		err = test_skcipher_vec_cfg(driver, DECRYPT, &vec, vec_name,
-					    &cfg, req, tsgls);
+					    cfg, req, tsgls);
 		if (err)
 			goto out;
 		cond_resched();
 	}
 	err = 0;
 out:
+	kfree(cfg);
 	kfree(vec.key);
 	kfree(vec.iv);
 	kfree(vec.ptext);
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers
  2019-06-17 13:23 [PATCH] crypto: testmgr - reduce stack usage in fuzzers Arnd Bergmann
@ 2019-06-17 14:04 ` Herbert Xu
  2019-06-17 14:10   ` Arnd Bergmann
  2019-06-17 17:20 ` Eric Biggers
  1 sibling, 1 reply; 10+ messages in thread
From: Herbert Xu @ 2019-06-17 14:04 UTC (permalink / raw)
  To: Arnd Bergmann, Kees Cook
  Cc: David S. Miller, Eric Biggers, Ard Biesheuvel, Vitaly Chikunov,
	Gilad Ben-Yossef, linux-crypto, linux-kernel

On Mon, Jun 17, 2019 at 03:23:02PM +0200, Arnd Bergmann wrote:
> On arm32, we get warnings about high stack usage in some of the functions:
> 
> crypto/testmgr.c:2269:12: error: stack frame size of 1032 bytes in function 'alg_test_aead' [-Werror,-Wframe-larger-than=]
> static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
>            ^
> crypto/testmgr.c:1693:12: error: stack frame size of 1312 bytes in function '__alg_test_hash' [-Werror,-Wframe-larger-than=]
> static int __alg_test_hash(const struct hash_testvec *vecs,
>            ^
> 
> On of the larger objects on the stack here is struct testvec_config, so
> change that to dynamic allocation.
> 
> Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
> Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
> Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I only compile-tested this, and it's not completely trivial, so please
> review carefully.

These structures are not meant to be that big.  I suspect something
has gone awry with the recent security conversions.

Kees?
-- 
Email: Herbert Xu <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] 10+ messages in thread

* Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers
  2019-06-17 14:04 ` Herbert Xu
@ 2019-06-17 14:10   ` Arnd Bergmann
  2019-06-17 14:24     ` Herbert Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2019-06-17 14:10 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Kees Cook, David S. Miller, Eric Biggers, Ard Biesheuvel,
	Vitaly Chikunov, Gilad Ben-Yossef,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
	Linux Kernel Mailing List

On Mon, Jun 17, 2019 at 4:04 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Jun 17, 2019 at 03:23:02PM +0200, Arnd Bergmann wrote:
> > On arm32, we get warnings about high stack usage in some of the functions:
> >
> > crypto/testmgr.c:2269:12: error: stack frame size of 1032 bytes in function 'alg_test_aead' [-Werror,-Wframe-larger-than=]
> > static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
> >            ^
> > crypto/testmgr.c:1693:12: error: stack frame size of 1312 bytes in function '__alg_test_hash' [-Werror,-Wframe-larger-than=]
> > static int __alg_test_hash(const struct hash_testvec *vecs,
> >            ^
> >
> > On of the larger objects on the stack here is struct testvec_config, so
> > change that to dynamic allocation.
> >
> > Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
> > Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
> > Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > I only compile-tested this, and it's not completely trivial, so please
> > review carefully.
>
> These structures are not meant to be that big.  I suspect something
> has gone awry with the recent security conversions.
>
> Kees?

I should have mentioned above that this happened with clang but not gcc.

We used to not be able to test with clang and KASAN. I had done some of
those tests in the past, but that was before Kees' nice cleanup, so the
potential stack overflow would already happen but not detected by the
compiler.

Both gcc and clang add a redzone around each stack variable that gets
passed into an 'extern' variable. The difference here is that with clang, the
size of that redzone is proportional to the size of the object, while with gcc
it is constant.

In most cases, this ends up in favor of clang (concerning the stack
warning size limit) because most variables are small, but here we have
a large stack object (two objects for the hash fuzzing) with a large redzone.

         Arnd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers
  2019-06-17 14:10   ` Arnd Bergmann
@ 2019-06-17 14:24     ` Herbert Xu
  2019-06-17 14:54       ` Arnd Bergmann
  0 siblings, 1 reply; 10+ messages in thread
From: Herbert Xu @ 2019-06-17 14:24 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Kees Cook, David S. Miller, Eric Biggers, Ard Biesheuvel,
	Vitaly Chikunov, Gilad Ben-Yossef,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
	Linux Kernel Mailing List

On Mon, Jun 17, 2019 at 04:10:44PM +0200, Arnd Bergmann wrote:
>
> In most cases, this ends up in favor of clang (concerning the stack
> warning size limit) because most variables are small, but here we have
> a large stack object (two objects for the hash fuzzing) with a large redzone.

Oh I missed the fact that there is another large stack variable
further up the stack.  So what happens if you just convert that
one and leave the shash descriptor alone?

Thanks,
-- 
Email: Herbert Xu <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] 10+ messages in thread

* Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers
  2019-06-17 14:24     ` Herbert Xu
@ 2019-06-17 14:54       ` Arnd Bergmann
  2019-06-17 14:56         ` Herbert Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2019-06-17 14:54 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Kees Cook, David S. Miller, Eric Biggers, Ard Biesheuvel,
	Vitaly Chikunov, Gilad Ben-Yossef,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
	Linux Kernel Mailing List

On Mon, Jun 17, 2019 at 4:24 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Jun 17, 2019 at 04:10:44PM +0200, Arnd Bergmann wrote:
> >
> > In most cases, this ends up in favor of clang (concerning the stack
> > warning size limit) because most variables are small, but here we have
> > a large stack object (two objects for the hash fuzzing) with a large redzone.
>
> Oh I missed the fact that there is another large stack variable
> further up the stack.  So what happens if you just convert that
> one and leave the shash descriptor alone?

Just converting the three testvec_config variables is what I originally
had in my patch. It got some configurations below the warning level,
but some others still had the problem. I considered sending two
separate patches, but as the symptom was the same, I just folded
it all into one patch that does the same thing in four functions.

       Arnd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers
  2019-06-17 14:54       ` Arnd Bergmann
@ 2019-06-17 14:56         ` Herbert Xu
  2019-06-17 15:22           ` Arnd Bergmann
  2019-06-17 15:28           ` David Laight
  0 siblings, 2 replies; 10+ messages in thread
From: Herbert Xu @ 2019-06-17 14:56 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Kees Cook, David S. Miller, Eric Biggers, Ard Biesheuvel,
	Vitaly Chikunov, Gilad Ben-Yossef,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
	Linux Kernel Mailing List

On Mon, Jun 17, 2019 at 04:54:16PM +0200, Arnd Bergmann wrote:
>
> Just converting the three testvec_config variables is what I originally
> had in my patch. It got some configurations below the warning level,
> but some others still had the problem. I considered sending two
> separate patches, but as the symptom was the same, I just folded
> it all into one patch that does the same thing in four functions.

Just curious, how bad is it with only moving testvec_config off
the stack?

Thanks,
-- 
Email: Herbert Xu <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] 10+ messages in thread

* Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers
  2019-06-17 14:56         ` Herbert Xu
@ 2019-06-17 15:22           ` Arnd Bergmann
  2019-06-17 15:28           ` David Laight
  1 sibling, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2019-06-17 15:22 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Kees Cook, David S. Miller, Eric Biggers, Ard Biesheuvel,
	Vitaly Chikunov, Gilad Ben-Yossef,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
	Linux Kernel Mailing List

On Mon, Jun 17, 2019 at 4:56 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Jun 17, 2019 at 04:54:16PM +0200, Arnd Bergmann wrote:
> >
> > Just converting the three testvec_config variables is what I originally
> > had in my patch. It got some configurations below the warning level,
> > but some others still had the problem. I considered sending two
> > separate patches, but as the symptom was the same, I just folded
> > it all into one patch that does the same thing in four functions.
>
> Just curious, how bad is it with only moving testvec_config off
> the stack?

I tried setting the warning limit to 256 now. On the original code I get
crypto/testmgr.c:2816:12: error: stack frame size of 984 bytes in
function 'alg_test_skcipher'
crypto/testmgr.c:2273:12: error: stack frame size of 1032 bytes in
function 'alg_test_aead'
crypto/testmgr.c:3267:12: error: stack frame size of 576 bytes in
function 'alg_test_crc32c'
crypto/testmgr.c:3811:12: error: stack frame size of 280 bytes in
function 'alg_test_akcipher'
crypto/testmgr.c:2798:12: error: stack frame size of 600 bytes in
function 'test_skcipher'
crypto/testmgr.c:2413:12: error: stack frame size of 352 bytes in
function 'test_skcipher_vec_cfg'
crypto/testmgr.c:2255:12: error: stack frame size of 600 bytes in
function 'test_aead'
crypto/testmgr.c:1823:12: error: stack frame size of 368 bytes in
function 'test_aead_vec_cfg'
crypto/testmgr.c:1694:12: error: stack frame size of 1408 bytes in
function '__alg_test_hash'

Just removing the testvec_config reduces the size of the largest three functions
by some 350 bytes, but I still get a warning for __alg_test_hash in some
configurations with the default 1024 byte limit:

crypto/testmgr.c:2837:12: error: stack frame size of 632 bytes in
function 'alg_test_skcipher'
crypto/testmgr.c:2287:12: error: stack frame size of 688 bytes in
function 'alg_test_aead'
crypto/testmgr.c:3288:12: error: stack frame size of 576 bytes in
function 'alg_test_crc32c'
crypto/testmgr.c:3832:12: error: stack frame size of 280 bytes in
function 'alg_test_akcipher'
crypto/testmgr.c:2819:12: error: stack frame size of 600 bytes in
function 'test_skcipher'
crypto/testmgr.c:2427:12: error: stack frame size of 352 bytes in
function 'test_skcipher_vec_cfg'
crypto/testmgr.c:2269:12: error: stack frame size of 600 bytes in
function 'test_aead'
crypto/testmgr.c:1830:12: error: stack frame size of 368 bytes in
function 'test_aead_vec_cfg'
crypto/testmgr.c:1701:12: error: stack frame size of 1088 bytes in
function '__alg_test_hash'

With the patch I posted, the last line goes down to 712:

crypto/testmgr.c:1709:12: error: stack frame size of 712 bytes in
function '__alg_test_hash'

In other subsystems, the numbers tend to be much smaller than in the crypto
code. I think a lot of that is inherent in the way the subsystem is designed,
but it also seems a little dangerous.

        Arnd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] crypto: testmgr - reduce stack usage in fuzzers
  2019-06-17 14:56         ` Herbert Xu
  2019-06-17 15:22           ` Arnd Bergmann
@ 2019-06-17 15:28           ` David Laight
  1 sibling, 0 replies; 10+ messages in thread
From: David Laight @ 2019-06-17 15:28 UTC (permalink / raw)
  To: 'Herbert Xu', Arnd Bergmann
  Cc: Kees Cook, David S. Miller, Eric Biggers, Ard Biesheuvel,
	Vitaly Chikunov, Gilad Ben-Yossef,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
	Linux Kernel Mailing List

From: Herbert Xu
> Sent: 17 June 2019 15:56
> On Mon, Jun 17, 2019 at 04:54:16PM +0200, Arnd Bergmann wrote:
> >
> > Just converting the three testvec_config variables is what I originally
> > had in my patch. It got some configurations below the warning level,
> > but some others still had the problem. I considered sending two
> > separate patches, but as the symptom was the same, I just folded
> > it all into one patch that does the same thing in four functions.
> 
> Just curious, how bad is it with only moving testvec_config off
> the stack?

This all reminds me of some code I wrote a long time ago (1984?)
that worked out the maximum stack use for a system by processing
all the .o files to find out which functions called which at what
stack depth and adding everything up.
That system had no indirect calls and no recursion, but the worst
stack use was always in diagnostic prints in obscure error paths.

My guess is that the same is true for the Linux kernel.
While getting rid of large on-stack buffers fixes the obvious cases
full analysis is needed to guarantee the stack won't overflow.
Doing that requires some method for determining the stack use
of indirect calls - which really requires knowing which type of
function is actually being called from each place.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers
  2019-06-17 13:23 [PATCH] crypto: testmgr - reduce stack usage in fuzzers Arnd Bergmann
  2019-06-17 14:04 ` Herbert Xu
@ 2019-06-17 17:20 ` Eric Biggers
  2019-06-17 20:05   ` Arnd Bergmann
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Biggers @ 2019-06-17 17:20 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Herbert Xu, David S. Miller, Eric Biggers, Ard Biesheuvel,
	Vitaly Chikunov, Gilad Ben-Yossef, linux-crypto, linux-kernel

On Mon, Jun 17, 2019 at 03:23:02PM +0200, Arnd Bergmann wrote:
> On arm32, we get warnings about high stack usage in some of the functions:
> 
> crypto/testmgr.c:2269:12: error: stack frame size of 1032 bytes in function 'alg_test_aead' [-Werror,-Wframe-larger-than=]
> static int alg_test_aead(const struct alg_test_desc *desc, const char *driver,
>            ^
> crypto/testmgr.c:1693:12: error: stack frame size of 1312 bytes in function '__alg_test_hash' [-Werror,-Wframe-larger-than=]
> static int __alg_test_hash(const struct hash_testvec *vecs,
>            ^
> 
> On of the larger objects on the stack here is struct testvec_config, so
> change that to dynamic allocation.
> 
> Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
> Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
> Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I only compile-tested this, and it's not completely trivial, so please
> review carefully.
> ---
>  crypto/testmgr.c | 61 +++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 6c28055d41ca..7928296cdcb3 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -1503,13 +1503,15 @@ static int test_hash_vec(const char *driver, const struct hash_testvec *vec,
>   * Generate a hash test vector from the given implementation.
>   * Assumes the buffers in 'vec' were already allocated.
>   */
> -static void generate_random_hash_testvec(struct crypto_shash *tfm,
> +static int generate_random_hash_testvec(struct crypto_shash *tfm,
>  					 struct hash_testvec *vec,
>  					 unsigned int maxkeysize,
>  					 unsigned int maxdatasize,
>  					 char *name, size_t max_namelen)
>  {
> -	SHASH_DESC_ON_STACK(desc, tfm);
> +	struct shash_desc *desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(tfm), GFP_KERNEL);
> +	if (!desc)
> +		return -ENOMEM;
>  
>  	/* Data */
>  	vec->psize = generate_random_length(maxdatasize);
> @@ -1541,6 +1543,10 @@ static void generate_random_hash_testvec(struct crypto_shash *tfm,
>  done:
>  	snprintf(name, max_namelen, "\"random: psize=%u ksize=%u\"",
>  		 vec->psize, vec->ksize);
> +
> +	kfree(desc);
> +
> +	return 0;
>  }

Instead of allocating the shash_desc here, can you allocate it in
test_hash_vs_generic_impl() and call it 'generic_desc'?  Then it would match
test_skcipher_vs_generic_impl() and test_aead_vs_generic_impl() which already
dynamically allocate their skcipher_request and aead_request, respectively.

>  
>  /*
> @@ -1565,7 +1571,7 @@ static int test_hash_vs_generic_impl(const char *driver,
>  	unsigned int i;
>  	struct hash_testvec vec = { 0 };
>  	char vec_name[64];
> -	struct testvec_config cfg;
> +	struct testvec_config *cfg;
>  	char cfgname[TESTVEC_CONFIG_NAMELEN];
>  	int err;
>  

Otherwise I guess this patch is fine for now, though there's still a lot of
stuff with nontrivial size on the stack (cfgname, vec_name, _generic_driver,
hash_testvec, plus the stuff in test_hash_vec_cfg).  There's also still a
testvec_config on the stack in test_{hash,skcipher,aead}_vec(); I assume you
didn't see a warning there only because it wasn't in combination with as much
other stuff on the stack.

I should probably have a go at refactoring this code to pack up most of this
stuff in *_params structures, which would then be dynamically allocated much
more easily.

- Eric

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers
  2019-06-17 17:20 ` Eric Biggers
@ 2019-06-17 20:05   ` Arnd Bergmann
  0 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2019-06-17 20:05 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Herbert Xu, David S. Miller, Eric Biggers, Ard Biesheuvel,
	Vitaly Chikunov, Gilad Ben-Yossef,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
	Linux Kernel Mailing List

On Mon, Jun 17, 2019 at 7:20 PM Eric Biggers <ebiggers@kernel.org> wrote:
> On Mon, Jun 17, 2019 at 03:23:02PM +0200, Arnd Bergmann wrote:
> > On arm32, we get warnings about high stack usage in some of the functions:
> >

> > @@ -1541,6 +1543,10 @@ static void generate_random_hash_testvec(struct crypto_shash *tfm,
> >  done:
> >       snprintf(name, max_namelen, "\"random: psize=%u ksize=%u\"",
> >                vec->psize, vec->ksize);
> > +
> > +     kfree(desc);
> > +
> > +     return 0;
> >  }
>
> Instead of allocating the   here, can you allocate it in
> test_hash_vs_generic_impl() and call it 'generic_desc'?  Then it would match
> test_skcipher_vs_generic_impl() and test_aead_vs_generic_impl() which already
> dynamically allocate their skcipher_request and aead_request, respectively.

Ok, good idea. I could not find an equivalent of skcipher_request_alloc()
and skcipher_request_free(), so I ended up open-coding those.

> > @@ -1565,7 +1571,7 @@ static int test_hash_vs_generic_impl(const char *driver,
> >       unsigned int i;
> >       struct hash_testvec vec = { 0 };
> >       char vec_name[64];
> > -     struct testvec_config cfg;
> > +     struct testvec_config *cfg;
> >       char cfgname[TESTVEC_CONFIG_NAMELEN];
> >       int err;
> >
>
> Otherwise I guess this patch is fine for now, though there's still a lot of
> stuff with nontrivial size on the stack (cfgname, vec_name, _generic_driver,
> hash_testvec, plus the stuff in test_hash_vec_cfg).  There's also still a
> testvec_config on the stack in test_{hash,skcipher,aead}_vec(); I assume you
> didn't see a warning there only because it wasn't in combination with as much
> other stuff on the stack.

Right, the stack usage for the other function is still several the hundreds of
bytes, but it falls under the radar of the 1024 byte warning limit.

> I should probably have a go at refactoring this code to pack up most of this
> stuff in *_params structures, which would then be dynamically allocated much
> more easily.

Makes sense. I'll leave that up to you, and will repost a set of two patches
based on your suggestion for testvec_config (unchanged) and
test_hash_vs_generic_impl, after build testing my current patches over night.

       Arnd

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-06-17 20:05 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 13:23 [PATCH] crypto: testmgr - reduce stack usage in fuzzers Arnd Bergmann
2019-06-17 14:04 ` Herbert Xu
2019-06-17 14:10   ` Arnd Bergmann
2019-06-17 14:24     ` Herbert Xu
2019-06-17 14:54       ` Arnd Bergmann
2019-06-17 14:56         ` Herbert Xu
2019-06-17 15:22           ` Arnd Bergmann
2019-06-17 15:28           ` David Laight
2019-06-17 17:20 ` Eric Biggers
2019-06-17 20:05   ` Arnd Bergmann

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