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 <a...@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
