mbox series

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

Message ID 20190412045742.1725-1-ebiggers@kernel.org (mailing list archive)
Headers show
Series crypto: fuzz algorithms against their generic implementation | expand

Message

Eric Biggers April 12, 2019, 4:57 a.m. UTC
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

 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(-)

Comments

Ard Biesheuvel April 12, 2019, 9:04 p.m. UTC | #1
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
>
Eric Biggers April 13, 2019, 2:40 a.m. UTC | #2
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
Herbert Xu April 18, 2019, 2:26 p.m. UTC | #3
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.
Horia Geanta April 26, 2019, 4:35 p.m. UTC | #4
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
Eric Biggers April 26, 2019, 4:54 p.m. UTC | #5
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
Horia Geanta April 27, 2019, 3:24 p.m. UTC | #6
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
Eric Biggers April 27, 2019, 5:02 p.m. UTC | #7
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
Horia Geanta May 2, 2019, 1:19 p.m. UTC | #8
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