On Fri, Apr 12, 2019 at 02:04:20PM -0700, Ard Biesheuvel wrote:
> On Thu, 11 Apr 2019 at 22:00, Eric Biggers <[email protected]> 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 <[email protected]>
> Tested-by: Ard Biesheuvel <[email protected]>
>
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
