| Message ID | 20190617132343.2678836-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: testmgr - reduce stack usage in fuzzers | expand |
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
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
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
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
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
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
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
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);
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(-) -- 2.20.0