linux-kernel - Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Jun 2019 10:20:10 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Biggers <ebiggers@...gle.com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Vitaly Chikunov <vt@...linux.org>,
        Gilad Ben-Yossef <gilad@...yossef.com>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers

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