Message ID | 20190412045742.1725-1-ebiggers@kernel.org (mailing list archive) |
---|---|
Headers | show |
Series | crypto: fuzz algorithms against their generic implementation | expand |
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> > crypto/842.c | 2 +- > crypto/adiantum.c | 2 +- > crypto/aegis128.c | 2 +- > crypto/aegis128l.c | 2 +- > crypto/aegis256.c | 2 +- > crypto/aes_generic.c | 2 +- > crypto/algboss.c | 8 +- > crypto/ansi_cprng.c | 2 +- > crypto/anubis.c | 2 +- > crypto/arc4.c | 2 +- > crypto/authenc.c | 2 +- > crypto/authencesn.c | 2 +- > crypto/blowfish_generic.c | 2 +- > crypto/camellia_generic.c | 2 +- > crypto/cast5_generic.c | 2 +- > crypto/cast6_generic.c | 2 +- > crypto/cbc.c | 2 +- > crypto/ccm.c | 2 +- > crypto/cfb.c | 2 +- > crypto/chacha20poly1305.c | 2 +- > crypto/chacha_generic.c | 2 +- > crypto/cmac.c | 2 +- > crypto/crc32_generic.c | 2 +- > crypto/crc32c_generic.c | 2 +- > crypto/crct10dif_generic.c | 2 +- > crypto/crypto_null.c | 2 +- > crypto/ctr.c | 2 +- > crypto/cts.c | 2 +- > crypto/deflate.c | 2 +- > crypto/des_generic.c | 2 +- > crypto/dh.c | 2 +- > crypto/drbg.c | 2 +- > crypto/ecb.c | 2 +- > crypto/ecdh.c | 2 +- > crypto/echainiv.c | 2 +- > crypto/fcrypt.c | 2 +- > crypto/fips.c | 2 +- > crypto/gcm.c | 2 +- > crypto/ghash-generic.c | 2 +- > crypto/hmac.c | 2 +- > crypto/jitterentropy-kcapi.c | 2 +- > crypto/keywrap.c | 2 +- > crypto/khazad.c | 2 +- > crypto/lrw.c | 2 +- > crypto/lz4.c | 2 +- > crypto/lz4hc.c | 2 +- > crypto/lzo-rle.c | 2 +- > crypto/lzo.c | 2 +- > crypto/md4.c | 2 +- > crypto/md5.c | 2 +- > crypto/michael_mic.c | 2 +- > crypto/morus1280.c | 2 +- > crypto/morus640.c | 2 +- > crypto/nhpoly1305.c | 2 +- > crypto/ofb.c | 2 +- > crypto/pcbc.c | 2 +- > crypto/pcrypt.c | 2 +- > crypto/poly1305_generic.c | 2 +- > crypto/rmd128.c | 2 +- > crypto/rmd160.c | 2 +- > crypto/rmd256.c | 2 +- > crypto/rmd320.c | 2 +- > crypto/rsa.c | 2 +- > crypto/salsa20_generic.c | 2 +- > crypto/seed.c | 2 +- > crypto/seqiv.c | 2 +- > crypto/serpent_generic.c | 2 +- > crypto/sha1_generic.c | 2 +- > crypto/sha256_generic.c | 2 +- > crypto/sha3_generic.c | 2 +- > crypto/sha512_generic.c | 2 +- > crypto/sm3_generic.c | 2 +- > crypto/sm4_generic.c | 2 +- > crypto/streebog_generic.c | 2 +- > crypto/tcrypt.c | 2 +- > crypto/tea.c | 2 +- > crypto/testmgr.c | 989 +++++++++++++++++++++++++++++++---- > crypto/testmgr.h | 22 +- > crypto/tgr192.c | 2 +- > crypto/twofish_generic.c | 2 +- > crypto/vmac.c | 2 +- > crypto/wp512.c | 2 +- > crypto/xcbc.c | 2 +- > crypto/xts.c | 2 +- > crypto/zstd.c | 2 +- > 85 files changed, 986 insertions(+), 197 deletions(-) > > -- > 2.21.0 >
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
On Thu, Apr 11, 2019 at 09:57:35PM -0700, Eric Biggers wrote: > > 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 > > crypto/842.c | 2 +- > crypto/adiantum.c | 2 +- > crypto/aegis128.c | 2 +- > crypto/aegis128l.c | 2 +- > crypto/aegis256.c | 2 +- > crypto/aes_generic.c | 2 +- > crypto/algboss.c | 8 +- > crypto/ansi_cprng.c | 2 +- > crypto/anubis.c | 2 +- > crypto/arc4.c | 2 +- > crypto/authenc.c | 2 +- > crypto/authencesn.c | 2 +- > crypto/blowfish_generic.c | 2 +- > crypto/camellia_generic.c | 2 +- > crypto/cast5_generic.c | 2 +- > crypto/cast6_generic.c | 2 +- > crypto/cbc.c | 2 +- > crypto/ccm.c | 2 +- > crypto/cfb.c | 2 +- > crypto/chacha20poly1305.c | 2 +- > crypto/chacha_generic.c | 2 +- > crypto/cmac.c | 2 +- > crypto/crc32_generic.c | 2 +- > crypto/crc32c_generic.c | 2 +- > crypto/crct10dif_generic.c | 2 +- > crypto/crypto_null.c | 2 +- > crypto/ctr.c | 2 +- > crypto/cts.c | 2 +- > crypto/deflate.c | 2 +- > crypto/des_generic.c | 2 +- > crypto/dh.c | 2 +- > crypto/drbg.c | 2 +- > crypto/ecb.c | 2 +- > crypto/ecdh.c | 2 +- > crypto/echainiv.c | 2 +- > crypto/fcrypt.c | 2 +- > crypto/fips.c | 2 +- > crypto/gcm.c | 2 +- > crypto/ghash-generic.c | 2 +- > crypto/hmac.c | 2 +- > crypto/jitterentropy-kcapi.c | 2 +- > crypto/keywrap.c | 2 +- > crypto/khazad.c | 2 +- > crypto/lrw.c | 2 +- > crypto/lz4.c | 2 +- > crypto/lz4hc.c | 2 +- > crypto/lzo-rle.c | 2 +- > crypto/lzo.c | 2 +- > crypto/md4.c | 2 +- > crypto/md5.c | 2 +- > crypto/michael_mic.c | 2 +- > crypto/morus1280.c | 2 +- > crypto/morus640.c | 2 +- > crypto/nhpoly1305.c | 2 +- > crypto/ofb.c | 2 +- > crypto/pcbc.c | 2 +- > crypto/pcrypt.c | 2 +- > crypto/poly1305_generic.c | 2 +- > crypto/rmd128.c | 2 +- > crypto/rmd160.c | 2 +- > crypto/rmd256.c | 2 +- > crypto/rmd320.c | 2 +- > crypto/rsa.c | 2 +- > crypto/salsa20_generic.c | 2 +- > crypto/seed.c | 2 +- > crypto/seqiv.c | 2 +- > crypto/serpent_generic.c | 2 +- > crypto/sha1_generic.c | 2 +- > crypto/sha256_generic.c | 2 +- > crypto/sha3_generic.c | 2 +- > crypto/sha512_generic.c | 2 +- > crypto/sm3_generic.c | 2 +- > crypto/sm4_generic.c | 2 +- > crypto/streebog_generic.c | 2 +- > crypto/tcrypt.c | 2 +- > crypto/tea.c | 2 +- > crypto/testmgr.c | 989 +++++++++++++++++++++++++++++++---- > crypto/testmgr.h | 22 +- > crypto/tgr192.c | 2 +- > crypto/twofish_generic.c | 2 +- > crypto/vmac.c | 2 +- > crypto/wp512.c | 2 +- > crypto/xcbc.c | 2 +- > crypto/xts.c | 2 +- > crypto/zstd.c | 2 +- > 85 files changed, 986 insertions(+), 197 deletions(-) All applied. Thanks.
On 4/12/2019 8:00 AM, Eric Biggers wrote: > 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. > I am seeing some errors in caam hardware driver. They are due to error code mismatch b/w generic algorithm implementation and what caam driver returns. Random skcipher tests for block cipher algorithms are expected to fail when input size is not a multiple of algorithm block size. Generic implementation returns -EINVAL. caam driver returns the status received from HW. This probably has to be fixed in caam driver, but I wonder if there's an agreement on what error code should be returned in every single case (since I'll have to do a N:M mapping b/w errors returned by HW and errors expected by crypto API). Should I take the generic S/W implementation as reference? Thanks, Horia
Hi Horia, On Fri, Apr 26, 2019 at 04:35:05PM +0000, Horia Geanta wrote: > On 4/12/2019 8:00 AM, Eric Biggers wrote: > > 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. > > > I am seeing some errors in caam hardware driver. > They are due to error code mismatch b/w generic algorithm implementation and > what caam driver returns. > > Random skcipher tests for block cipher algorithms are expected to fail when > input size is not a multiple of algorithm block size. > Generic implementation returns -EINVAL. > caam driver returns the status received from HW. > > This probably has to be fixed in caam driver, but I wonder if there's an > agreement on what error code should be returned in every single case (since I'll > have to do a N:M mapping b/w errors returned by HW and errors expected by crypto > API). > Should I take the generic S/W implementation as reference? > > Thanks, > Horia Yes, use the generic driver as a reference. I don't understand why you're saying there are so many cases to handle, though. The only error cases I'd expect to actually be encountered during the tests are invalid input lengths and invalid key lengths, where you should return -EINVAL. There may be other errors your driver could theoretically produce, but I wouldn't expect them to be encountered during the tests unless there are testmgr, driver, or hardware bugs. But remember you must always return a -errno code, not a driver-specific code. - Eric
On 4/26/2019 7:54 PM, Eric Biggers wrote: > Hi Horia, > > On Fri, Apr 26, 2019 at 04:35:05PM +0000, Horia Geanta wrote: >> On 4/12/2019 8:00 AM, Eric Biggers wrote: >>> 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. >>> >> I am seeing some errors in caam hardware driver. >> They are due to error code mismatch b/w generic algorithm implementation and >> what caam driver returns. >> >> Random skcipher tests for block cipher algorithms are expected to fail when >> input size is not a multiple of algorithm block size. >> Generic implementation returns -EINVAL. >> caam driver returns the status received from HW. >> >> This probably has to be fixed in caam driver, but I wonder if there's an >> agreement on what error code should be returned in every single case (since I'll >> have to do a N:M mapping b/w errors returned by HW and errors expected by crypto >> API). >> Should I take the generic S/W implementation as reference? >> >> Thanks, >> Horia > > Yes, use the generic driver as a reference. I don't understand why you're > saying there are so many cases to handle, though. The only error cases I'd > expect to actually be encountered during the tests are invalid input lengths and > invalid key lengths, where you should return -EINVAL. There may be other errors There's at least one more in testmgr: -EBADMSG. > your driver could theoretically produce, but I wouldn't expect them to be > encountered during the tests unless there are testmgr, driver, or hardware bugs. > Is the error code matching a crypto API requirement or a testmgr requirement? I think testmgr doesn't cover all possible failures. Thus if somebody wants to make sure the implementation is _fully_ compliant (and not only passing testmgr tests), a lot of effort will be required. > But remember you must always return a -errno code, not a driver-specific code. > Correct, I'll fix this. Thanks, Horia
On Sat, Apr 27, 2019 at 03:24:38PM +0000, Horia Geanta wrote: > On 4/26/2019 7:54 PM, Eric Biggers wrote: > > Hi Horia, > > > > On Fri, Apr 26, 2019 at 04:35:05PM +0000, Horia Geanta wrote: > >> On 4/12/2019 8:00 AM, Eric Biggers wrote: > >>> 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. > >>> > >> I am seeing some errors in caam hardware driver. > >> They are due to error code mismatch b/w generic algorithm implementation and > >> what caam driver returns. > >> > >> Random skcipher tests for block cipher algorithms are expected to fail when > >> input size is not a multiple of algorithm block size. > >> Generic implementation returns -EINVAL. > >> caam driver returns the status received from HW. > >> > >> This probably has to be fixed in caam driver, but I wonder if there's an > >> agreement on what error code should be returned in every single case (since I'll > >> have to do a N:M mapping b/w errors returned by HW and errors expected by crypto > >> API). > >> Should I take the generic S/W implementation as reference? > >> > >> Thanks, > >> Horia > > > > Yes, use the generic driver as a reference. I don't understand why you're > > saying there are so many cases to handle, though. The only error cases I'd > > expect to actually be encountered during the tests are invalid input lengths and > > invalid key lengths, where you should return -EINVAL. There may be other errors > There's at least one more in testmgr: -EBADMSG. AEADs must return -EBADMSG when the authentication tag is wrong, but you said it was an skcipher algorithm. Which algorithm are you talking about, exactly? > > > your driver could theoretically produce, but I wouldn't expect them to be > > encountered during the tests unless there are testmgr, driver, or hardware bugs. > > > Is the error code matching a crypto API requirement or a testmgr requirement? > > I think testmgr doesn't cover all possible failures. Thus if somebody wants to > make sure the implementation is _fully_ compliant (and not only passing testmgr > tests), a lot of effort will be required. > Everything testmgr tests for is an "API requirement". There are also API requirements that testmgr doen't yet test for, e.g. for skciphers that the source data is not modified unless it coincides with the destination data. However with regards to failures, as I see it the only failures which *must* be handled consistently are those that apply to every implementation. I think this only includes cases where the input is bad, e.g. invalid key or message length, or authentication tag mismatch. If you also have implementation specific failures, e.g. your hardware randomly stopped working or something, then for them you may choose appropriate error codes from errno.h. > > But remember you must always return a -errno code, not a driver-specific code. > > > Correct, I'll fix this. > > Thanks, > Horia
On 4/27/2019 8:03 PM, Eric Biggers wrote: > On Sat, Apr 27, 2019 at 03:24:38PM +0000, Horia Geanta wrote: >> On 4/26/2019 7:54 PM, Eric Biggers wrote: >>> Hi Horia, >>> >>> On Fri, Apr 26, 2019 at 04:35:05PM +0000, Horia Geanta wrote: >>>> On 4/12/2019 8:00 AM, Eric Biggers wrote: >>>>> 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. >>>>> >>>> I am seeing some errors in caam hardware driver. >>>> They are due to error code mismatch b/w generic algorithm implementation and >>>> what caam driver returns. >>>> >>>> Random skcipher tests for block cipher algorithms are expected to fail when >>>> input size is not a multiple of algorithm block size. >>>> Generic implementation returns -EINVAL. >>>> caam driver returns the status received from HW. >>>> >>>> This probably has to be fixed in caam driver, but I wonder if there's an >>>> agreement on what error code should be returned in every single case (since I'll >>>> have to do a N:M mapping b/w errors returned by HW and errors expected by crypto >>>> API). >>>> Should I take the generic S/W implementation as reference? >>>> >>>> Thanks, >>>> Horia >>> >>> Yes, use the generic driver as a reference. I don't understand why you're >>> saying there are so many cases to handle, though. The only error cases I'd >>> expect to actually be encountered during the tests are invalid input lengths and >>> invalid key lengths, where you should return -EINVAL. There may be other errors >> There's at least one more in testmgr: -EBADMSG. > > AEADs must return -EBADMSG when the authentication tag is wrong, but you said it > was an skcipher algorithm. Which algorithm are you talking about, exactly? > The context in which I started the discussion was indeed skcipher failure. However, now I am trying to make sure all types of algorithm implementations are returning the expected error codes. >> >>> your driver could theoretically produce, but I wouldn't expect them to be >>> encountered during the tests unless there are testmgr, driver, or hardware bugs. >>> >> Is the error code matching a crypto API requirement or a testmgr requirement? >> >> I think testmgr doesn't cover all possible failures. Thus if somebody wants to >> make sure the implementation is _fully_ compliant (and not only passing testmgr >> tests), a lot of effort will be required. >> > > Everything testmgr tests for is an "API requirement". There are also API > requirements that testmgr doen't yet test for, e.g. for skciphers that the > source data is not modified unless it coincides with the destination data. > > However with regards to failures, as I see it the only failures which *must* be > handled consistently are those that apply to every implementation. I think this > only includes cases where the input is bad, e.g. invalid key or message length, > or authentication tag mismatch. If you also have implementation specific Ok, the list is not as long as I suspected. > failures, e.g. your hardware randomly stopped working or something, then for > them you may choose appropriate error codes from errno.h. > This makes sense. Thanks, Horia