| Message ID | 20190617132343.2678836-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| 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?
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,
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,
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
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)
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
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
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(-)