MIME-Version: 1.0
References: <20190123224926.250525-1-ebiggers@kernel.org> <20190124084818.GA650@sol.localdomain>
In-Reply-To: <20190124084818.GA650@sol.localdomain>
From:   Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Thu, 24 Jan 2019 09:50:35 +0100
Message-ID: <CAKv+Gu9zridWAYO3cR_KAAbupWfLd+FAMmcgAMuYE5Lgxoek-w@mail.gmail.com>
Subject: Re: [RFC/RFT PATCH 00/15] crypto: improved skcipher, aead, and hash tests
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "Jason A . Donenfeld" <Jason@zx2c4.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Thu, 24 Jan 2019 at 09:48, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Jan 23, 2019 at 02:49:11PM -0800, Eric Biggers wrote:
> > Hello,
> >
> > Crypto algorithms must produce the same output for the same input
> > regardless of data layout, i.e. how the src and dst scatterlists are
> > divided into chunks and how each chunk is aligned.  Request flags such
> > as CRYPTO_TFM_REQ_MAY_SLEEP must not affect the result either.
> >
> > However, testing of this currently has many gaps.  For example,
> > individual algorithms are responsible for providing their own chunked
> > test vectors.  But many don't bother to do this or test only one or two
> > cases, providing poor test coverage.  Also, other things such as
> > misaligned IVs and CRYPTO_TFM_REQ_MAY_SLEEP are never tested at all.
> >
> > Test code is also duplicated between the chunked and non-chunked cases,
> > making it difficult to make other improvements.
> >
> > To improve the situation, this patch series basically moves the chunk
> > descriptions into the testmgr itself so that they are shared by all
> > algorithms.  However, it's done in an extensible way via a new struct
> > 'testvec_config', which describes not just the scaled chunk lengths but
> > also all other aspects of the crypto operation besides the data itself
> > such as the buffer alignments, the request flags, whether the operation
> > is in-place or not, the IV alignment, and for hash algorithms when to do
> > each update() and when to use finup() vs. final() vs. digest().
> >
> > Then, this patch series makes skcipher, aead, and hash algorithms be
> > tested against a list of default testvec_configs, replacing the current
> > test code.  This improves overall test coverage, without reducing test
> > performance too much.  Note that the test vectors themselves are not
> > changed, except for removing the chunk lists.
> >
> > This series also adds randomized fuzz tests, enabled by a new kconfig
> > option intended for developer use only, where skcipher, aead, and hash
> > algorithms are tested against many randomly generated testvec_configs.
> > This provides much more comprehensive test coverage.
> >
> > These improved tests have already found many bugs.  Patches 1-7 fix the
> > bugs found so far (*).  However, I've only tested implementations that I
> > can easily test.  There will be more bugs found, especially in
> > hardware-specific drivers.  Anyone reading this can help by applying
> > these patches on your system (especially if it's non-x86 and/or has
> > crypto accelerators), enabling CONFIG_CRYPTO_MANAGER_EXTRA_TESTS, and
> > reporting or fixing any test failures.
>
> On an arm64 system with the crypto extensions, crct10dif-arm64-ce and ccm-aes-ce
> are failing too:
>
> [    1.632623] alg: hash: crct10dif-arm64-ce test failed (wrong result) on test vector 0, cfg="init+update+update+final two even splits"
> [   15.377921] alg: aead: ccm-aes-ce decryption failed with err -74 on test vector 11, cfg="uneven misaligned splits, may sleep"
>
> Ard, I'll fix these when I have time but feel free to get to them first.
>

Hi Eric,

Thanks for yet another round of cleanup

I'll look into these, but I'd like to clarify one thing first.

IIUC, you are trying to deal with the case where a single scatterlist
element describes a range that strides two pages, and I wonder if that
is a valid use of scatterlists in the first place.

Herbert?