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] [day] [month] [year] [list]

Message-ID: <CAK8P3a04d+xLWjmyDAn80EabkWOEnFXw-R=DGEfns37oPNXtRA@mail.gmail.com>
Date:   Mon, 17 Jun 2019 22:05:19 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Eric Biggers <ebiggers@...nel.org>
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>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] crypto: testmgr - reduce stack usage in fuzzers

On Mon, Jun 17, 2019 at 7:20 PM Eric Biggers <ebiggers@...nel.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