Re: [PATCH v2 0/7] crypto: fuzz algorithms against their generic implementation

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Apr 12, 2019 at 02:04:20PM -0700, Ard Biesheuvel wrote:
> On Thu, 11 Apr 2019 at 22:00, Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > Hello,
> >
> > In the crypto API, all implementations of each algorithm are supposed to
> > produce the same results.  However, testing of this is currently limited
> > to the list of test vectors hardcoded for each algorithm.  Although
> > after recent improvements the self-tests do much more with each test
> > vector, hardcoded test vectors can never cover all cases.
> >
> > This series improves the situation by making the self-tests
> > automatically generate random test vectors using the corresponding
> > generic implementation, then run them against the algorithm under test.
> > This detects bugs where the implementations don't match.
> >
> > This has already found many bugs and inconsistencies, including an
> > integer overflow bug in the x86_64 implementation of Poly1305.
> >
> > These new fuzz tests are behind CONFIG_CRYPTO_MANAGER_EXTRA_TESTS.
> >
> > Patch 1-6 are the testmgr changes themselves.  Patch 7 makes the generic
> > implementations be registered earlier so that they're available when
> > optimized implementations are being tested, when both are built-in.
> > Note that even after this, for many algorithms it's still possible to
> > make the generic implementation unset or modular.  Thus a missing
> > generic implementation just causes the comparison tests to be skipped
> > with a warning; they aren't failed.
> >
> > So far I've tested all generic, x86, arm, and arm64 algorithms, plus
> > some PowerPC algorithms.  I have not tested hardware drivers.  I
> > encourage people to run the tests on drivers and other architectures, as
> > they will find more bugs.
> >
> > This can also be found in git at:
> >
> >         URL: https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
> >         Branch: cryptofuzz-vs-generic
> >
> > Changed since v1:
> >
> >     - Make cryptomgr use arch_initcall(), so we don't rely on the order
> >       in which the object files are linked.
> >
> >     - Show the expected error code when a test fails due to the wrong
> >       error code being returned.
> >
> >     - Generate zero-length associated data more often for AEADs
> >       (about 1/4 of the time rather than about 1/256 of the time).
> >
> >     - A few other minor cleanups.
> >
> > Eric Biggers (7):
> >   crypto: testmgr - expand ability to test for errors
> >   crypto: testmgr - identify test vectors by name rather than number
> >   crypto: testmgr - add helpers for fuzzing against generic
> >     implementation
> >   crypto: testmgr - fuzz hashes against their generic implementation
> >   crypto: testmgr - fuzz skciphers against their generic implementation
> >   crypto: testmgr - fuzz AEADs against their generic implementation
> >   crypto: run initcalls for generic implementations earlier
> >
> 
> This looks alright to me, but I have to admit I did not look at every
> patch in great detail. I did put it through kernelci, though, and it
> built and booted fine on all systems.
> 
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 

Thanks Ard.  Note that the logs from the kernelci run do show new test failures
in two drivers:

	alg: skcipher: ecb-aes-s5p encryption failed on test vector \"random: len=0 klen=32\"; expected_error=0, actual_error=-22, cfg=\"random: inplace use_final src_divs=[<flush>73.70%@alignmask+4049, 11.97%@+3977, <reimport,nosimd>3.65%@+4013, <reimport>10.68%@alignmask+12] iv_offset=4\"
	alg: skcipher: cbc-aes-s5p encryption failed on test vector \"random: len=0 klen=32\"; expected_error=0, actual_error=-22, cfg=\"random: use_finup src_divs=[<reimport>100.0%@+22] dst_divs=[100.0%@+258]\"
	alg: skcipher: blocksize for ctr-aes-s5p (16) doesn't match generic impl (1)

	alg: skcipher: ecb-aes-rk encryption failed on test vector \"random: len=0 klen=32\"; expected_error=0, actual_error=-22, cfg=\"random: inplace use_digest src_divs=[100.0%@alignmask+2107] iv_offset=38\"


So, unlike the generic implementations, the s5p-sss driver doesn't allow empty
messages for AES-ECB and AES-CBC, and it sets cra_blocksize for AES-CTR to 16
bytes rather than 1.  (It's supposed to be set to 1 for all stream ciphers.)

And the Rockchip crypto driver doesn't allow empty messages for AES-ECB.

None of these appear super important, but the tests seem to be working as
intended to find them, and they'll need to be fixed.

- Eric