Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 29 Aug 2019 10:58:19 -0500
From:   Eric Biggers <ebiggers@kernel.org>
To:     Christophe Leroy <christophe.leroy@c-s.fr>
Cc:     linux-crypto@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 13/15] crypto: testmgr - convert hash testing to use
 testvec_configs
Message-ID: <20190829155819.GA14558@zzz.localdomain>
Mail-Followup-To: Christophe Leroy <christophe.leroy@c-s.fr>,
        linux-crypto@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        linux-kernel@vger.kernel.org
References: <20190201075150.18644-1-ebiggers@kernel.org>
 <20190201075150.18644-14-ebiggers@kernel.org>
 <e1ce6e9f-dc20-79bc-cd53-faff92481f7a@c-s.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e1ce6e9f-dc20-79bc-cd53-faff92481f7a@c-s.fr>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Thu, Aug 29, 2019 at 05:32:46PM +0200, Christophe Leroy wrote:
> Hi Eric,
> 
> 
> Le 01/02/2019 ? 08:51, Eric Biggers a ?crit?:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Convert alg_test_hash() to use the new test framework, adding a list of
> > testvec_configs to test by default.  When the extra self-tests are
> > enabled, randomly generated testvec_configs are tested as well.
> > 
> > This improves hash test coverage mainly because now all algorithms have
> > a variety of data layouts tested, whereas before each algorithm was
> > responsible for declaring its own chunked test cases which were often
> > missing or provided poor test coverage.  The new code also tests both
> > the MAY_SLEEP and !MAY_SLEEP cases and buffers that cross pages.
> > 
> > This already found bugs in the hash walk code and in the arm32 and arm64
> > implementations of crct10dif.
> > 
> > I removed the hash chunked test vectors that were the same as
> > non-chunked ones, but left the ones that were unique.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >   crypto/testmgr.c | 795 ++++++++++++++++++++---------------------------
> >   crypto/testmgr.h | 107 +------
> >   2 files changed, 352 insertions(+), 550 deletions(-)
> > 
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index 7638090ff1b0a..96aa268ff4184 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> 
> [...]
> 
> 
> > -static int __test_hash(struct crypto_ahash *tfm,
> > -		       const struct hash_testvec *template, unsigned int tcount,
> > -		       enum hash_test test_type, const int align_offset)
> > +static int test_hash_vec_cfg(const char *driver,
> > +			     const struct hash_testvec *vec,
> > +			     unsigned int vec_num,
> > +			     const struct testvec_config *cfg,
> > +			     struct ahash_request *req,
> > +			     struct test_sglist *tsgl,
> > +			     u8 *hashstate)
> >   {
> > -	const char *algo = crypto_tfm_alg_driver_name(crypto_ahash_tfm(tfm));
> > -	size_t digest_size = crypto_ahash_digestsize(tfm);
> > -	unsigned int i, j, k, temp;
> > -	struct scatterlist sg[8];
> > -	char *result;
> > -	char *key;
> > -	struct ahash_request *req;
> > -	struct crypto_wait wait;
> > -	void *hash_buff;
> > -	char *xbuf[XBUFSIZE];
> > -	int ret = -ENOMEM;
> > -
> > -	result = kmalloc(digest_size, GFP_KERNEL);
> > -	if (!result)
> > -		return ret;
> > -	key = kmalloc(MAX_KEYLEN, GFP_KERNEL);
> > -	if (!key)
> > -		goto out_nobuf;
> > -	if (testmgr_alloc_buf(xbuf))
> > -		goto out_nobuf;
> > +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> > +	const unsigned int alignmask = crypto_ahash_alignmask(tfm);
> > +	const unsigned int digestsize = crypto_ahash_digestsize(tfm);
> > +	const unsigned int statesize = crypto_ahash_statesize(tfm);
> > +	const u32 req_flags = CRYPTO_TFM_REQ_MAY_BACKLOG | cfg->req_flags;
> > +	const struct test_sg_division *divs[XBUFSIZE];
> > +	DECLARE_CRYPTO_WAIT(wait);
> > +	struct kvec _input;
> > +	struct iov_iter input;
> > +	unsigned int i;
> > +	struct scatterlist *pending_sgl;
> > +	unsigned int pending_len;
> > +	u8 result[HASH_MAX_DIGESTSIZE + TESTMGR_POISON_LEN];
> 
> Before this patch, result was allocated with kmalloc().
> Now, result is in the stack. Is there a reason for this change ?
> 
> Due to this change, the talitos driver fails when using CONFIG_VMAP_STACK,
> because result is not dma-able anymore.
> 

Yes, moving 'result' to the stack simplified the code slightly and also helps
detect buggy drivers that assume that 'result' is DMA-able.  Note that some
"real" API users also put 'result' on the stack, as the API allows it.
This is not new; the only new thing is that the tests now test it.

> 
> How should this be fixed ?
> 

Take a look at how other drivers have fixed this issue in the past, e.g.
commit c19650d6ea9 ("crypto: caam - fix DMA mapping of stack memory").
Seems that you need to DMA into a temporary buffer.

- Eric