On Dec 17, 2020, at 02:16, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > We will need to refactor this - cloning the entire driver and just > replacing aes-ni with aes-kl is a maintenance nightmare. > > Please refer to the arm64 tree for an example how to combine chaining > mode routines implemented in assembler with different implementations > of the core AES transforms (aes-modes.S is combined with either > aes-ce.S or aes-neon.S to produce two different drivers) I just post v2 [1]. PATCH9 [2] refactors some glue code out of AES-NI to prepare AES-KL. [ Past a few months were not fully spent on this but it took a while to address comments and to debug test cases. ] > ... >> diff --git a/arch/x86/crypto/aeskl-intel_glue.c b/arch/x86/crypto/aeskl-intel_glue.c >> new file mode 100644 >> index 000000000000..9e3f900ad4af >> --- /dev/null >> +++ b/arch/x86/crypto/aeskl-intel_glue.c >> @@ -0,0 +1,697 @@ > ... >> +static void aeskl_encrypt(struct crypto_tfm *tfm, u8 *dst, const u8 *src) >> +{ >> + struct crypto_aes_ctx *ctx = aes_ctx(crypto_tfm_ctx(tfm)); >> + int err = 0; >> + >> + if (!crypto_simd_usable()) >> + return; >> + > > It is clear that AES-KL cannot be handled by a fallback algorithm, > given that the key is no longer available. But that doesn't mean that > you can just give up like this. > > This basically implies that we cannot expose the cipher interface at > all, and so AES-KL can only be used by callers that use the > asynchronous interface, which rules out 802.11, s/w kTLS, macsec and > kerberos. I made not to expose the synchronous interface, in v2. > This ties in to a related discussion that is going on about when to > allow kernel mode SIMD. I am currently investigating whether we can > change the rules a bit so that crypto_simd_usable() is guaranteed to > be true. I saw your series [3]. Yes, I’m very interested in it. >> +static int ecb_encrypt(struct skcipher_request *req) >> +{ >> + struct crypto_skcipher *tfm; >> + struct crypto_aes_ctx *ctx; >> + struct skcipher_walk walk; >> + unsigned int nbytes; >> + int err; >> + >> + tfm = crypto_skcipher_reqtfm(req); >> + ctx = aes_ctx(crypto_skcipher_ctx(tfm)); >> + >> + err = skcipher_walk_virt(&walk, req, true); >> + if (err) >> + return err; >> + >> + while ((nbytes = walk.nbytes)) { >> + unsigned int len = nbytes & AES_BLOCK_MASK; >> + const u8 *src = walk.src.virt.addr; >> + u8 *dst = walk.dst.virt.addr; >> + >> + kernel_fpu_begin(); >> + if (unlikely(ctx->key_length == AES_KEYSIZE_192)) >> + aesni_ecb_enc(ctx, dst, src, len); > > Could we please use a proper fallback here, and relay the entire request? I made a change like this in v2: +static int ecb_encrypt(struct skcipher_request *req) +{ + struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req); + + if (likely(keylength(crypto_skcipher_ctx(tfm)) != AES_KEYSIZE_192)) + return ecb_crypt_common(req, aeskl_ecb_enc); + else + return ecb_crypt_common(req, aesni_ecb_enc); +} >> + else >> + err = __aeskl_ecb_enc(ctx, dst, src, len); >> + kernel_fpu_end(); >> + >> + if (err) { >> + skcipher_walk_done(&walk, nbytes & (AES_BLOCK_SIZE - 1)); > > This doesn't look right. The skcipher scatterlist walker may have a > live kmap() here so you can't just return. I’ve added a preparatory patch [4] to deal with cases like this. Thanks, Chang [1] https://lore.kernel.org/lkml/20210514201508.27967-1-chang.seok.bae@xxxxxxxxx/ [2] https://lore.kernel.org/lkml/20210514201508.27967-10-chang.seok.bae@xxxxxxxxx/ [3] https://lore.kernel.org/lkml/20201218170106.23280-1-ardb@xxxxxxxxxx/ [4] https://lore.kernel.org/lkml/20210514201508.27967-9-chang.seok.bae@xxxxxxxxx/