running kernel mode SIMD with softirqs disabled
Subsystems (2)
v4 cover running kernel mode SIMD with softirqs disabled
This is a follow-up to [0], but given that the arm64 architectural pieces have been merged for arm64, the only remaining changes are crypto specific. Therefore, the audience has been reduced to those people who are likely to care about these specifics.
Patch #1 addresses an issue in the skcipher walker which doesn't handle zero sized AEAD inputs entirely consistently, which is uncovered by the change in patch #7.
Patches #2 and #3 add some sanity checks to the public AEAD and skcipher APIs to limit their availibility to either task or softirq context (which is the only way in which they are currently being used). Adding this restriction permits the arm64 crypto code to get rid of all scalar fallbacks, given that on this architecture, softirqs are no longer served while the SIMD unit is being used in kernel mode, which means that the scalar fallbacks are never needed. These are removed in the remaining 4 patches.
Changes since v3:
- clarify the nature of the issue addressed by patch #1, and apply the same fix to the skcipher walker
- update patches #2 and #3 so that the failures can be observed by the crypto stats code
[0] https://lore.kernel.org/linux-arm-kernel/[email protected]/
Ard Biesheuvel (7):
crypto: skcipher - handle zero sized inputs correctly
crypto: aead - disallow en/decrypt for non-task or non-softirq context
crypto: skcipher - disallow en/decrypt for non-task or non-softirq
context
crypto: arm64/gcm-aes-ce - remove non-SIMD fallback path
crypto: arm64/aes-neonbs - stop using SIMD helper for skciphers
crypto: arm64/aes-ce - stop using SIMD helper for skciphers
crypto: arm64/aes-ccm - remove non-SIMD fallback path
arch/arm64/crypto/Kconfig | 6 - arch/arm64/crypto/aes-ce-ccm-core.S | 1 + arch/arm64/crypto/aes-ce-ccm-glue.c | 183 +++++------------ arch/arm64/crypto/aes-glue.c | 102 ++-------- arch/arm64/crypto/aes-neonbs-glue.c | 122 +----------- arch/arm64/crypto/ghash-ce-glue.c | 209 +++++--------------- crypto/aead.c | 12 +- crypto/skcipher.c | 16 +- 8 files changed, 150 insertions(+), 501 deletions(-)
Signature · 1 line
2.20.1
PATCH v4 1/7 crypto: skcipher - handle zero sized inputs correctly
There are corner cases where skcipher_walk_aead_encrypt() may be invoked with a zero sized input, which is not rejected by the walker code, but results in the skcipher_walk structure to not be fully initialized. This will leave stale values in its page and buffer members, which will be subsequently passed to kfree() or free_page() by skcipher_walk_done(), resulting in a crash if those routines fail to identify them as in valid inputs.
Fix this by setting page and buffer to NULL even if the size of the input is zero. Note that for AEAD encryption in particular, a zero sized input could be a valid and meaningful use of the API, given the support for associated authenticated data (AAD), which gets reflected in the authentication tag as well.
For symmetry, apply the same fix to the plain skcipher walker code, even though in that case, no meaningful usage scenarios are known for zero sized inputs.
--- crypto/skcipher.c | 4 ++++ 1 file changed, 4 insertions(+)
crypto/skcipher.c +4 -0
@@ -455,6 +455,8 @@ static int skcipher_walk_skcipher(struct skcipher_walk *walk, walk->nbytes =0 ; walk->iv = req->iv; walk->oiv = req->iv; + walk->buffer =NULL ; + walk->page =NULL ;if (unlikely (!walk->total))return 0 ;
@@ -511,6 +513,8 @@ static int skcipher_walk_aead_common(struct skcipher_walk *walk, walk->nbytes =0 ; walk->iv = req->iv; walk->oiv = req->iv; + walk->buffer =NULL ; + walk->page =NULL ;if (unlikely (!walk->total))return 0 ;
Signature · 1 line
2.20.1
PATCH v4 2/7 crypto: aead - disallow en/decrypt for non-task or non-softirq context
In reply to Ard BiesheuvelIn order to ensure that kernel mode SIMD routines will not need a scalar fallback if they run with softirqs disabled, disallow any use of the AEAD encrypt and decrypt routines from outside of task or softirq context.
--- crypto/aead.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
crypto/aead.c +10 -2
@@ -88,7 +88,11 @@ int crypto_aead_encrypt(struct aead_request *req)int ret;crypto_stats_get (alg); -if (crypto_aead_get_flags (aead) &CRYPTO_TFM_NEED_KEY ) +if (!(alg->cra_flags &CRYPTO_ALG_ASYNC ) && +WARN_ONCE (!in_task () && !in_serving_softirq (), +"synchronous call from invalid context\n" )) + ret = -EBUSY ; +else if (crypto_aead_get_flags (aead) &CRYPTO_TFM_NEED_KEY ) ret = -ENOKEY ;else ret =crypto_aead_alg (aead)->encrypt (req);
@@ -105,7 +109,11 @@ int crypto_aead_decrypt(struct aead_request *req)int ret;crypto_stats_get (alg); -if (crypto_aead_get_flags (aead) &CRYPTO_TFM_NEED_KEY ) +if (!(alg->cra_flags &CRYPTO_ALG_ASYNC ) && +WARN_ONCE (!in_task () && !in_serving_softirq (), +"synchronous call from invalid context\n" )) + ret = -EBUSY ; +else if (crypto_aead_get_flags (aead) &CRYPTO_TFM_NEED_KEY ) ret = -ENOKEY ;else if (req->cryptlen <crypto_aead_authsize (aead)) ret = -EINVAL ;
Signature · 1 line
2.20.1
Herbert Xu
maintainerRe: [PATCH v4 2/7] crypto: aead - disallow en/decrypt for non-task or non-softirq context
crypto/aead.c int crypto_aead_encrypt(struct aead_request *req) · line 94
Quoted 7 lines · Ard Biesheuvel
crypto/aead.c +4
-1crypto_stats_get (alg); -if (crypto_aead_get_flags (aead) &CRYPTO_TFM_NEED_KEY ) +if (!(alg->cra_flags &CRYPTO_ALG_ASYNC ) && +WARN_ONCE (!in_task () && !in_serving_softirq (), +"synchronous call from invalid context\n" )) + ret = -EBUSY ;
I don't think we've ever supported crypto in hard IRQ contexts. So this should be done regardless of ASYNC.
Then again, do we really need this since the assumption has always been that the crypto API can only be invoked in user or softirq context?
Cheers,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v4 2/7] crypto: aead - disallow en/decrypt for non-task or non-softirq context
Quoted 10 lines · Herbert Xu
crypto/aead.c int crypto_aead_encrypt(struct aead_request *req) · line 94
Quoted 7 lines · Ard Biesheuvel
crypto_stats_get(alg);crypto/aead.c +4
-1-if (crypto_aead_get_flags (aead) &CRYPTO_TFM_NEED_KEY ) +if (!(alg->cra_flags &CRYPTO_ALG_ASYNC ) && +WARN_ONCE (!in_task () && !in_serving_softirq (), +"synchronous call from invalid context\n" )) + ret = -EBUSY ;
Quoted 3 lines
I don't think we've ever supported crypto in hard IRQ contexts. So this should be done regardless of ASYNC.
OK.
Quoted 4 lines
Then again, do we really need this since the assumption has always been that the crypto API can only be invoked in user or softirq context?
With this series applied, some of the arm64 accelerated s/w implementations will no longer work correctly when this rule is violated, and so it would be nice to have a sanity check somewhere. And policing rules like these is best done in generic code, right?
So if we do need to check this, we should check it here. If we don't, then we can drop these patches.
Herbert Xu
maintainerRe: [PATCH v4 2/7] crypto: aead - disallow en/decrypt for non-task or non-softirq context
Quoted 3 lines · Ard Biesheuvel
So if we do need to check this, we should check it here. If we don't, then we can drop these patches.
Historically other things would break in nasty ways if you tried to do crypto in hard IRQ contexts, e.g., overwritten kmap slots back when we had individual slots for each context, but I don't think we've ever found anyone crazy enough to do that to warrant a run-time check.
I'd just leave it out for now.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v4 2/7] crypto: aead - disallow en/decrypt for non-task or non-softirq context
Quoted 6 lines · Herbert Xu
Quoted 3 lines · Ard Biesheuvel
So if we do need to check this, we should check it here. If we don't, then we can drop these patches.
Quoted 8 lines
Historically other things would break in nasty ways if you tried to do crypto in hard IRQ contexts, e.g., overwritten kmap slots back when we had individual slots for each context, but I don't think we've ever found anyone crazy enough to do that to warrant a run-time check.
I'd just leave it out for now.
Fair enough. Would you like me to resend the series with these patches left out Or are you ok to just take the remaining ones (assuming there are no issues reported with those)?
PATCH v4 3/7 crypto: skcipher - disallow en/decrypt for non-task or non-softirq context
In reply to Ard BiesheuvelIn order to ensure that kernel mode SIMD routines will not need a scalar fallback if they run with softirqs disabled, disallow any use of the skcipher encrypt and decrypt routines from outside of task or softirq context.
--- crypto/skcipher.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
crypto/skcipher.c +10 -2
@@ -628,7 +628,11 @@ int crypto_skcipher_encrypt(struct skcipher_request *req)int ret;crypto_stats_get (alg); -if (crypto_skcipher_get_flags (tfm) &CRYPTO_TFM_NEED_KEY ) +if (!(alg->cra_flags &CRYPTO_ALG_ASYNC ) && +WARN_ONCE (!in_task () && !in_serving_softirq (), +"synchronous call from invalid context\n" )) + ret = -EBUSY ; +else if (crypto_skcipher_get_flags (tfm) &CRYPTO_TFM_NEED_KEY ) ret = -ENOKEY ;else ret =crypto_skcipher_alg (tfm)->encrypt (req);
@@ -645,7 +649,11 @@ int crypto_skcipher_decrypt(struct skcipher_request *req)int ret;crypto_stats_get (alg); -if (crypto_skcipher_get_flags (tfm) &CRYPTO_TFM_NEED_KEY ) +if (!(alg->cra_flags &CRYPTO_ALG_ASYNC ) && +WARN_ONCE (!in_task () && !in_serving_softirq (), +"synchronous call from invalid context\n" )) + ret = -EBUSY ; +else if (crypto_skcipher_get_flags (tfm) &CRYPTO_TFM_NEED_KEY ) ret = -ENOKEY ;else ret =crypto_skcipher_alg (tfm)->decrypt (req);
Signature · 1 line
2.20.1
PATCH v4 4/7 crypto: arm64/gcm-aes-ce - remove non-SIMD fallback path
In reply to Ard BiesheuvelNow that kernel mode SIMD is guaranteed to be available when executing in task or softirq context, we no longer need scalar fallbacks to use when the NEON is unavailable. So get rid of them.
--- arch/arm64/crypto/ghash-ce-glue.c | 209 +++++--------------- 1 file changed, 51 insertions(+), 158 deletions(-)
arch/arm64/crypto/ghash-ce-glue.c +51 -158
@@ -362,84 +362,36 @@ static int gcm_encrypt(struct aead_request *req) err =skcipher_walk_aead_encrypt (&walk, req, false); -if (likely (crypto_simd_usable ())) { -do { -const u8 *src = walk.src.virt.addr; -u8 *dst = walk.dst.virt.addr; -int nbytes = walk.nbytes; - - tag = (u8 *)&lengths; - -if (unlikely (nbytes >0 && nbytes <AES_BLOCK_SIZE )) { - src = dst =memcpy (buf +sizeof (buf) - nbytes, - src, nbytes); - }else if (nbytes < walk.total) { - nbytes &= ~(AES_BLOCK_SIZE -1 ); - tag =NULL ; - } - -kernel_neon_begin (); -pmull_gcm_encrypt (nbytes, dst, src, ctx->ghash_key.h, - dg, iv, ctx->aes_key.key_enc, nrounds, - tag); -kernel_neon_end (); - -if (unlikely (!nbytes)) -break ; - -if (unlikely (nbytes >0 && nbytes <AES_BLOCK_SIZE )) -memcpy (walk.dst.virt.addr, - buf +sizeof (buf) - nbytes, nbytes); - - err =skcipher_walk_done (&walk, walk.nbytes - nbytes); - }while (walk.nbytes); - }else { -while (walk.nbytes >=AES_BLOCK_SIZE ) { -int blocks = walk.nbytes /AES_BLOCK_SIZE ; -const u8 *src = walk.src.virt.addr; -u8 *dst = walk.dst.virt.addr; -int remaining = blocks; - -do { -aes_encrypt (&ctx->aes_key, buf, iv); -crypto_xor_cpy (dst, src, buf,AES_BLOCK_SIZE ); -crypto_inc (iv,AES_BLOCK_SIZE ); - - dst +=AES_BLOCK_SIZE ; - src +=AES_BLOCK_SIZE ; - }while (--remaining >0 ); - -ghash_do_update (blocks, dg, walk.dst.virt.addr, - &ctx->ghash_key,NULL ); - - err =skcipher_walk_done (&walk, - walk.nbytes %AES_BLOCK_SIZE ); - } - -/* handle the tail */ -if (walk.nbytes) { -aes_encrypt (&ctx->aes_key, buf, iv); +do { +const u8 *src = walk.src.virt.addr; +u8 *dst = walk.dst.virt.addr; +int nbytes = walk.nbytes; -crypto_xor_cpy (walk.dst.virt.addr, walk.src.virt.addr, - buf, walk.nbytes); + tag = (u8 *)&lengths; -memcpy (buf, walk.dst.virt.addr, walk.nbytes); -memset (buf + walk.nbytes,0 ,sizeof (buf) - walk.nbytes); +if (unlikely (nbytes >0 && nbytes <AES_BLOCK_SIZE )) { + src = dst =memcpy (buf +sizeof (buf) - nbytes, + src, nbytes); + }else if (nbytes < walk.total) { + nbytes &= ~(AES_BLOCK_SIZE -1 ); + tag =NULL ; } - tag = (u8 *)&lengths; -ghash_do_update (1 , dg, tag, &ctx->ghash_key, - walk.nbytes ? buf :NULL ); +kernel_neon_begin (); +pmull_gcm_encrypt (nbytes, dst, src, ctx->ghash_key.h, + dg, iv, ctx->aes_key.key_enc, nrounds, + tag); +kernel_neon_end (); -if (walk.nbytes) - err =skcipher_walk_done (&walk,0 ); +if (unlikely (!nbytes)) +break ; -put_unaligned_be64 (dg[1 ], tag); -put_unaligned_be64 (dg[0 ], tag +8 ); -put_unaligned_be32 (1 , iv +GCM_IV_SIZE ); -aes_encrypt (&ctx->aes_key, iv, iv); -crypto_xor (tag, iv,AES_BLOCK_SIZE ); - } +if (unlikely (nbytes >0 && nbytes <AES_BLOCK_SIZE )) +memcpy (walk.dst.virt.addr, + buf +sizeof (buf) - nbytes, nbytes); + + err =skcipher_walk_done (&walk, walk.nbytes - nbytes); + }while (walk.nbytes);if (err)return err;
@@ -464,6 +416,7 @@ static int gcm_decrypt(struct aead_request *req)u64 dg[2 ] = {};be128 lengths;u8 *tag; +int ret;int err; lengths.a =cpu_to_be64 (req->assoclen *8 );
@@ -481,101 +434,41 @@ static int gcm_decrypt(struct aead_request *req) err =skcipher_walk_aead_decrypt (&walk, req, false); -if (likely (crypto_simd_usable ())) { -int ret; - -do { -const u8 *src = walk.src.virt.addr; -u8 *dst = walk.dst.virt.addr; -int nbytes = walk.nbytes; - - tag = (u8 *)&lengths; - -if (unlikely (nbytes >0 && nbytes <AES_BLOCK_SIZE )) { - src = dst =memcpy (buf +sizeof (buf) - nbytes, - src, nbytes); - }else if (nbytes < walk.total) { - nbytes &= ~(AES_BLOCK_SIZE -1 ); - tag =NULL ; - } - -kernel_neon_begin (); - ret =pmull_gcm_decrypt (nbytes, dst, src, - ctx->ghash_key.h, - dg, iv, ctx->aes_key.key_enc, - nrounds, tag, otag, authsize); -kernel_neon_end (); - -if (unlikely (!nbytes)) -break ; - -if (unlikely (nbytes >0 && nbytes <AES_BLOCK_SIZE )) -memcpy (walk.dst.virt.addr, - buf +sizeof (buf) - nbytes, nbytes); - - err =skcipher_walk_done (&walk, walk.nbytes - nbytes); - }while (walk.nbytes); - -if (err) -return err; -if (ret) -return -EBADMSG ; - }else { -while (walk.nbytes >=AES_BLOCK_SIZE ) { -int blocks = walk.nbytes /AES_BLOCK_SIZE ; -const u8 *src = walk.src.virt.addr; -u8 *dst = walk.dst.virt.addr; - -ghash_do_update (blocks, dg, walk.src.virt.addr, - &ctx->ghash_key,NULL ); - -do { -aes_encrypt (&ctx->aes_key, buf, iv); -crypto_xor_cpy (dst, src, buf,AES_BLOCK_SIZE ); -crypto_inc (iv,AES_BLOCK_SIZE ); - - dst +=AES_BLOCK_SIZE ; - src +=AES_BLOCK_SIZE ; - }while (--blocks >0 ); +do { +const u8 *src = walk.src.virt.addr; +u8 *dst = walk.dst.virt.addr; +int nbytes = walk.nbytes; - err =skcipher_walk_done (&walk, - walk.nbytes %AES_BLOCK_SIZE ); - } + tag = (u8 *)&lengths; -/* handle the tail */ -if (walk.nbytes) { -memcpy (buf, walk.src.virt.addr, walk.nbytes); -memset (buf + walk.nbytes,0 ,sizeof (buf) - walk.nbytes); +if (unlikely (nbytes >0 && nbytes <AES_BLOCK_SIZE )) { + src = dst =memcpy (buf +sizeof (buf) - nbytes, + src, nbytes); + }else if (nbytes < walk.total) { + nbytes &= ~(AES_BLOCK_SIZE -1 ); + tag =NULL ; } - tag = (u8 *)&lengths; -ghash_do_update (1 , dg, tag, &ctx->ghash_key, - walk.nbytes ? buf :NULL ); - -if (walk.nbytes) { -aes_encrypt (&ctx->aes_key, buf, iv); +kernel_neon_begin (); + ret =pmull_gcm_decrypt (nbytes, dst, src, ctx->ghash_key.h, + dg, iv, ctx->aes_key.key_enc, + nrounds, tag, otag, authsize); +kernel_neon_end (); -crypto_xor_cpy (walk.dst.virt.addr, walk.src.virt.addr, - buf, walk.nbytes); +if (unlikely (!nbytes)) +break ; - err =skcipher_walk_done (&walk,0 ); - } +if (unlikely (nbytes >0 && nbytes <AES_BLOCK_SIZE )) +memcpy (walk.dst.virt.addr, + buf +sizeof (buf) - nbytes, nbytes); -if (err) -return err; + err =skcipher_walk_done (&walk, walk.nbytes - nbytes); + }while (walk.nbytes); -put_unaligned_be64 (dg[1 ], tag); -put_unaligned_be64 (dg[0 ], tag +8 ); -put_unaligned_be32 (1 , iv +GCM_IV_SIZE ); -aes_encrypt (&ctx->aes_key, iv, iv); -crypto_xor (tag, iv,AES_BLOCK_SIZE ); +if (err) +return err; -if (crypto_memneq (tag, otag, authsize)) { -memzero_explicit (tag,AES_BLOCK_SIZE ); -return -EBADMSG ; - } - } -return 0 ; +return ret ? -EBADMSG :0 ; }static struct aead_alg gcm_aes_alg = {
Signature · 1 line
2.20.1
PATCH v4 5/7 crypto: arm64/aes-neonbs - stop using SIMD helper for skciphers
In reply to Ard BiesheuvelCalls into the skcipher API can only occur from contexts where the SIMD unit is available, so there is no need for the SIMD helper.
--- arch/arm64/crypto/Kconfig | 2 - arch/arm64/crypto/aes-neonbs-glue.c | 122 ++------------------ 2 files changed, 9 insertions(+), 115 deletions(-)
arch/arm64/crypto/Kconfig +0 -2
@@ -122,8 +122,6 @@ config CRYPTO_AES_ARM64_BS depends on KERNEL_MODE_NEON select CRYPTO_SKCIPHER select CRYPTO_AES_ARM64_NEON_BLK - select CRYPTO_AES_ARM64 select CRYPTO_LIB_AES - select CRYPTO_SIMD endif
arch/arm64/crypto/aes-neonbs-glue.c +9 -113
196 lines not on this page · raw message
Signature · 1 line
2.20.1
PATCH v4 6/7 crypto: arm64/aes-ce - stop using SIMD helper for skciphers
In reply to Ard BiesheuvelCalls into the skcipher API can only occur from contexts where the SIMD unit is available, so there is no need for the SIMD helper.
--- arch/arm64/crypto/Kconfig | 4 - arch/arm64/crypto/aes-glue.c | 102 +++----------------- 2 files changed, 13 insertions(+), 93 deletions(-)
arch/arm64/crypto/Kconfig +0 -4
@@ -88,16 +88,12 @@ config CRYPTO_AES_ARM64_CE_BLK depends on KERNEL_MODE_NEON select CRYPTO_SKCIPHER select CRYPTO_AES_ARM64_CE - select CRYPTO_AES_ARM64 - select CRYPTO_SIMD config CRYPTO_AES_ARM64_NEON_BLK tristate "AES in ECB/CBC/CTR/XTS modes using NEON instructions" depends on KERNEL_MODE_NEON select CRYPTO_SKCIPHER - select CRYPTO_AES_ARM64 select CRYPTO_LIB_AES - select CRYPTO_SIMD config CRYPTO_CHACHA20_NEON tristate "ChaCha20, XChaCha20, and XChaCha12 stream ciphers using NEON instructions"
arch/arm64/crypto/aes-glue.c +13 -89
179 lines not on this page · raw message
Signature · 1 line
2.20.1
PATCH v4 7/7 crypto: arm64/aes-ccm - remove non-SIMD fallback path
In reply to Ard BiesheuvelAES/CCM on arm64 is implemented as a synchronous AEAD, and so it is guaranteed by the API that it is only invoked in task or softirq context. Since softirqs are now only handled when the SIMD is not being used in the task context that was interrupted to service the softirq, we no longer need a fallback path. Let's remove it.
--- arch/arm64/crypto/aes-ce-ccm-core.S | 1 + arch/arm64/crypto/aes-ce-ccm-glue.c | 183 ++++++-------------- 2 files changed, 53 insertions(+), 131 deletions(-)
arch/arm64/crypto/aes-ce-ccm-core.S +1 -0
@@ -124,6 +124,7 @@ SYM_FUNC_START(ce_aes_ccm_final) SYM_FUNC_END(ce_aes_ccm_final) .macro aes_ccm_do_crypt,enc + cbz x2, 5f ldr x8, [x6, #8] /* load lower ctr */ ld1 {v0.16b}, [x5] /* load mac */ CPU_LE( rev x8, x8 ) /* keep swabbed ctr in reg */
arch/arm64/crypto/aes-ce-ccm-glue.c +52 -131
225 lines not on this page · raw message
Signature · 1 line
2.20.1