| Message ID | 20190123224926.250525-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | crypto: improved skcipher, aead, and hash tests | expand |
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. > > This patch series can also be found in git at > https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git > branch "testmgr-improvements". > > (*) Except that many AEADs incorrectly change aead_request::base.tfm. > I've left fixing that for later patches. > > Eric Biggers (15): > crypto: aegis - fix handling chunked inputs > crypto: morus - fix handling chunked inputs > crypto: x86/aegis - fix handling chunked inputs and MAY_SLEEP > crypto: x86/morus - fix handling chunked inputs and MAY_SLEEP > crypto: x86/aesni-gcm - fix crash on empty plaintext > crypto: ahash - fix another early termination in hash walk > crypto: arm64/aes-neonbs - fix returning final keystream block > crypto: testmgr - add testvec_config struct and helper functions > crypto: testmgr - introduce CONFIG_CRYPTO_MANAGER_EXTRA_TESTS > crypto: testmgr - implement random testvec_config generation > crypto: testmgr - convert skcipher testing to use testvec_configs > crypto: testmgr - convert aead testing to use testvec_configs > crypto: testmgr - convert hash testing to use testvec_configs > crypto: testmgr - check for skcipher_request corruption > crypto: testmgr - check for aead_request corruption > > arch/arm64/crypto/aes-neonbs-core.S | 8 +- > arch/x86/crypto/aegis128-aesni-glue.c | 38 +- > arch/x86/crypto/aegis128l-aesni-glue.c | 38 +- > arch/x86/crypto/aegis256-aesni-glue.c | 38 +- > arch/x86/crypto/aesni-intel_glue.c | 13 +- > arch/x86/crypto/morus1280_glue.c | 40 +- > arch/x86/crypto/morus640_glue.c | 39 +- > crypto/Kconfig | 10 + > crypto/aegis128.c | 14 +- > crypto/aegis128l.c | 14 +- > crypto/aegis256.c | 14 +- > crypto/ahash.c | 14 +- > crypto/morus1280.c | 13 +- > crypto/morus640.c | 13 +- > crypto/testmgr.c | 2552 +++++++++++++----------- > crypto/testmgr.h | 407 +--- > 16 files changed, 1558 insertions(+), 1707 deletions(-) > > -- > 2.20.1.321.g9e740568ce-goog >
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?
On Thu, Jan 24, 2019 at 09:50:35AM +0100, Ard Biesheuvel wrote: > > 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? Yes it is valid. IIRC the network stack may generate such a scatterlist. Thanks,
On Thu, 24 Jan 2019 at 10:24, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Thu, Jan 24, 2019 at 09:50:35AM +0100, Ard Biesheuvel wrote: > > > > 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? > > Yes it is valid. IIRC the network stack may generate such a > scatterlist. > OK, so how many adjacent physical pages is a scatterlist entry permitted to cover? Unbounded?
On Thu, Jan 24, 2019 at 11:16:23AM +0100, Ard Biesheuvel wrote: > > OK, so how many adjacent physical pages is a scatterlist entry > permitted to cover? Unbounded? Well the network stack is limited by packet size so it's less than 64K for practical purposes. However, the API itself doesn't specify a limit as such. Cheers,
On Thu, Jan 24, 2019 at 05:23:59PM +0800, Herbert Xu wrote: > On Thu, Jan 24, 2019 at 09:50:35AM +0100, Ard Biesheuvel wrote: > > > > 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? > > Yes it is valid. IIRC the network stack may generate such a > scatterlist. > Also it can easily happen with kmalloced buffers, e.g. buf = kmalloc(10000, GFP_KERNEL); [...] sg_init_one(&sg, buf, 10000); - Eric
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. This patch series can also be found in git at https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git branch "testmgr-improvements". (*) Except that many AEADs incorrectly change aead_request::base.tfm. I've left fixing that for later patches. Eric Biggers (15): crypto: aegis - fix handling chunked inputs crypto: morus - fix handling chunked inputs crypto: x86/aegis - fix handling chunked inputs and MAY_SLEEP crypto: x86/morus - fix handling chunked inputs and MAY_SLEEP crypto: x86/aesni-gcm - fix crash on empty plaintext crypto: ahash - fix another early termination in hash walk crypto: arm64/aes-neonbs - fix returning final keystream block crypto: testmgr - add testvec_config struct and helper functions crypto: testmgr - introduce CONFIG_CRYPTO_MANAGER_EXTRA_TESTS crypto: testmgr - implement random testvec_config generation crypto: testmgr - convert skcipher testing to use testvec_configs crypto: testmgr - convert aead testing to use testvec_configs crypto: testmgr - convert hash testing to use testvec_configs crypto: testmgr - check for skcipher_request corruption crypto: testmgr - check for aead_request corruption arch/arm64/crypto/aes-neonbs-core.S | 8 +- arch/x86/crypto/aegis128-aesni-glue.c | 38 +- arch/x86/crypto/aegis128l-aesni-glue.c | 38 +- arch/x86/crypto/aegis256-aesni-glue.c | 38 +- arch/x86/crypto/aesni-intel_glue.c | 13 +- arch/x86/crypto/morus1280_glue.c | 40 +- arch/x86/crypto/morus640_glue.c | 39 +- crypto/Kconfig | 10 + crypto/aegis128.c | 14 +- crypto/aegis128l.c | 14 +- crypto/aegis256.c | 14 +- crypto/ahash.c | 14 +- crypto/morus1280.c | 13 +- crypto/morus640.c | 13 +- crypto/testmgr.c | 2552 +++++++++++++----------- crypto/testmgr.h | 407 +--- 16 files changed, 1558 insertions(+), 1707 deletions(-)