1. Patchew
  2. linux
  3. View series

[PATCH] crypto: x86/aesni - Update aesni_set_key() to return void

Chang S. Bae posted 1 patch 1 year, 11 months ago 
arch/x86/crypto/aesni-intel_asm.S  | 5 ++---
arch/x86/crypto/aesni-intel_glue.c | 6 +++---
2 files changed, 5 insertions(+), 6 deletions(-)
[PATCH] crypto: x86/aesni - Update aesni_set_key() to return void
Posted by Chang S. Bae 1 year, 11 months ago 
The aesni_set_key() implementation has no error case, yet its prototype
specifies to return an error code.

Modify the function prototype to return void and adjust the related code.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: linux-crypto@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
Previously, Eric identified a similar case in my AES-KL code [1]. Then,
this parallel issue was realized.

[1]: https://lore.kernel.org/lkml/20230607053558.GC941@sol.localdomain/
---
 arch/x86/crypto/aesni-intel_asm.S  | 5 ++---
 arch/x86/crypto/aesni-intel_glue.c | 6 +++---
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_asm.S b/arch/x86/crypto/aesni-intel_asm.S
index 411d8c83e88a..7ecb55cae3d6 100644
--- a/arch/x86/crypto/aesni-intel_asm.S
+++ b/arch/x86/crypto/aesni-intel_asm.S
@@ -1820,8 +1820,8 @@ SYM_FUNC_START_LOCAL(_key_expansion_256b)
 SYM_FUNC_END(_key_expansion_256b)
 
 /*
- * int aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
- *                   unsigned int key_len)
+ * void aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
+ *                    unsigned int key_len)
  */
 SYM_FUNC_START(aesni_set_key)
 	FRAME_BEGIN
@@ -1926,7 +1926,6 @@ SYM_FUNC_START(aesni_set_key)
 	sub $0x10, UKEYP
 	cmp TKEYP, KEYP
 	jb .Ldec_key_loop
-	xor AREG, AREG
 #ifndef __x86_64__
 	popl KEYP
 #endif
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index b1d90c25975a..c807b2f48ea3 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -87,8 +87,8 @@ static inline void *aes_align_addr(void *addr)
 	return PTR_ALIGN(addr, AESNI_ALIGN);
 }
 
-asmlinkage int aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
-			     unsigned int key_len);
+asmlinkage void aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
+			      unsigned int key_len);
 asmlinkage void aesni_enc(const void *ctx, u8 *out, const u8 *in);
 asmlinkage void aesni_dec(const void *ctx, u8 *out, const u8 *in);
 asmlinkage void aesni_ecb_enc(struct crypto_aes_ctx *ctx, u8 *out,
@@ -241,7 +241,7 @@ static int aes_set_key_common(struct crypto_aes_ctx *ctx,
 		err = aes_expandkey(ctx, in_key, key_len);
 	else {
 		kernel_fpu_begin();
-		err = aesni_set_key(ctx, in_key, key_len);
+		aesni_set_key(ctx, in_key, key_len);
 		kernel_fpu_end();
 	}
 
-- 
2.34.1
[PATCH v2 0/2] crypto: x86/aesni - Simplify AES key expansion code
Posted by Chang S. Bae 1 year, 10 months ago 
Previously, V1 [1] was posted to update the aesni_set_key() prototype to
remove an unnecessary error code return type.

During the review process, it was discovered that both aes_expandkey()
and the AES-NI glue code redundantly check the key size. V2 includes a
cleanup to invoke aes_expandkey() immediately if AES-NI is unusable.

Thanks,
Chang

[1]: https://lore.kernel.org/lkml/20240311213232.128240-1-chang.seok.bae@intel.com/

Chang S. Bae (2):
  crypto: x86/aesni - Rearrange AES key size check
  crypto: x86/aesni - Update aesni_set_key() to return void

 arch/x86/crypto/aesni-intel_asm.S  |  5 ++---
 arch/x86/crypto/aesni-intel_glue.c | 24 +++++++++++-------------
 2 files changed, 13 insertions(+), 16 deletions(-)


base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
-- 
2.34.1
Re: [PATCH v2 0/2] crypto: x86/aesni - Simplify AES key expansion code
Posted by Herbert Xu 1 year, 10 months ago 
On Fri, Mar 22, 2024 at 04:04:57PM -0700, Chang S. Bae wrote:
> Previously, V1 [1] was posted to update the aesni_set_key() prototype to
> remove an unnecessary error code return type.
> 
> During the review process, it was discovered that both aes_expandkey()
> and the AES-NI glue code redundantly check the key size. V2 includes a
> cleanup to invoke aes_expandkey() immediately if AES-NI is unusable.
> 
> Thanks,
> Chang
> 
> [1]: https://lore.kernel.org/lkml/20240311213232.128240-1-chang.seok.bae@intel.com/
> 
> Chang S. Bae (2):
>   crypto: x86/aesni - Rearrange AES key size check
>   crypto: x86/aesni - Update aesni_set_key() to return void
> 
>  arch/x86/crypto/aesni-intel_asm.S  |  5 ++---
>  arch/x86/crypto/aesni-intel_glue.c | 24 +++++++++++-------------
>  2 files changed, 13 insertions(+), 16 deletions(-)
> 
> 
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> -- 
> 2.34.1

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH v2 1/2] crypto: x86/aesni - Rearrange AES key size check
Posted by Chang S. Bae 1 year, 10 months ago 
aes_expandkey() already includes an AES key size check. If AES-NI is
unusable, invoke the function without the size check.

Also, use aes_check_keylen() instead of open code.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: linux-crypto@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
V2 <- V1:
* Add as a new patch.
---
 arch/x86/crypto/aesni-intel_glue.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index b1d90c25975a..8b3b17b065ad 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -233,18 +233,16 @@ static int aes_set_key_common(struct crypto_aes_ctx *ctx,
 {
 	int err;
 
-	if (key_len != AES_KEYSIZE_128 && key_len != AES_KEYSIZE_192 &&
-	    key_len != AES_KEYSIZE_256)
-		return -EINVAL;
-
 	if (!crypto_simd_usable())
-		err = aes_expandkey(ctx, in_key, key_len);
-	else {
-		kernel_fpu_begin();
-		err = aesni_set_key(ctx, in_key, key_len);
-		kernel_fpu_end();
-	}
+		return aes_expandkey(ctx, in_key, key_len);
 
+	err = aes_check_keylen(key_len);
+	if (err)
+		return err;
+
+	kernel_fpu_begin();
+	err = aesni_set_key(ctx, in_key, key_len);
+	kernel_fpu_end();
 	return err;
 }
 
-- 
2.34.1
[PATCH v2 2/2] crypto: x86/aesni - Update aesni_set_key() to return void
Posted by Chang S. Bae 1 year, 10 months ago 
The aesni_set_key() implementation has no error case, yet its prototype
specifies to return an error code.

Modify the function prototype to return void and adjust the related code.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Eric Biggers <ebiggers@google.com>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: linux-crypto@vger.kernel.org
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
V2 <- V1:
* Ensure not to return an uninitialized value. (Ard Biesheuvel)

Previously, Eric identified a similar case in my AES-KL code [1]. Then,
this parallel issue was realized.
[1]: https://lore.kernel.org/lkml/20230607053558.GC941@sol.localdomain/
---
 arch/x86/crypto/aesni-intel_asm.S  | 5 ++---
 arch/x86/crypto/aesni-intel_glue.c | 8 ++++----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_asm.S b/arch/x86/crypto/aesni-intel_asm.S
index 411d8c83e88a..7ecb55cae3d6 100644
--- a/arch/x86/crypto/aesni-intel_asm.S
+++ b/arch/x86/crypto/aesni-intel_asm.S
@@ -1820,8 +1820,8 @@ SYM_FUNC_START_LOCAL(_key_expansion_256b)
 SYM_FUNC_END(_key_expansion_256b)
 
 /*
- * int aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
- *                   unsigned int key_len)
+ * void aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
+ *                    unsigned int key_len)
  */
 SYM_FUNC_START(aesni_set_key)
 	FRAME_BEGIN
@@ -1926,7 +1926,6 @@ SYM_FUNC_START(aesni_set_key)
 	sub $0x10, UKEYP
 	cmp TKEYP, KEYP
 	jb .Ldec_key_loop
-	xor AREG, AREG
 #ifndef __x86_64__
 	popl KEYP
 #endif
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 8b3b17b065ad..0ea3abaaa645 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -87,8 +87,8 @@ static inline void *aes_align_addr(void *addr)
 	return PTR_ALIGN(addr, AESNI_ALIGN);
 }
 
-asmlinkage int aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
-			     unsigned int key_len);
+asmlinkage void aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
+			      unsigned int key_len);
 asmlinkage void aesni_enc(const void *ctx, u8 *out, const u8 *in);
 asmlinkage void aesni_dec(const void *ctx, u8 *out, const u8 *in);
 asmlinkage void aesni_ecb_enc(struct crypto_aes_ctx *ctx, u8 *out,
@@ -241,9 +241,9 @@ static int aes_set_key_common(struct crypto_aes_ctx *ctx,
 		return err;
 
 	kernel_fpu_begin();
-	err = aesni_set_key(ctx, in_key, key_len);
+	aesni_set_key(ctx, in_key, key_len);
 	kernel_fpu_end();
-	return err;
+	return 0;
 }
 
 static int aes_set_key(struct crypto_tfm *tfm, const u8 *in_key,
-- 
2.34.1
Re: [PATCH] crypto: x86/aesni - Update aesni_set_key() to return void
Posted by Ard Biesheuvel 1 year, 11 months ago 
On Mon, 11 Mar 2024 at 22:48, Chang S. Bae <chang.seok.bae@intel.com> wrote:
>
> The aesni_set_key() implementation has no error case, yet its prototype
> specifies to return an error code.
>
> Modify the function prototype to return void and adjust the related code.
>
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: linux-crypto@vger.kernel.org
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> Previously, Eric identified a similar case in my AES-KL code [1]. Then,
> this parallel issue was realized.
>
> [1]: https://lore.kernel.org/lkml/20230607053558.GC941@sol.localdomain/
> ---
>  arch/x86/crypto/aesni-intel_asm.S  | 5 ++---
>  arch/x86/crypto/aesni-intel_glue.c | 6 +++---
>  2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/crypto/aesni-intel_asm.S b/arch/x86/crypto/aesni-intel_asm.S
> index 411d8c83e88a..7ecb55cae3d6 100644
> --- a/arch/x86/crypto/aesni-intel_asm.S
> +++ b/arch/x86/crypto/aesni-intel_asm.S
> @@ -1820,8 +1820,8 @@ SYM_FUNC_START_LOCAL(_key_expansion_256b)
>  SYM_FUNC_END(_key_expansion_256b)
>
>  /*
> - * int aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
> - *                   unsigned int key_len)
> + * void aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
> + *                    unsigned int key_len)
>   */
>  SYM_FUNC_START(aesni_set_key)
>         FRAME_BEGIN
> @@ -1926,7 +1926,6 @@ SYM_FUNC_START(aesni_set_key)
>         sub $0x10, UKEYP
>         cmp TKEYP, KEYP
>         jb .Ldec_key_loop
> -       xor AREG, AREG
>  #ifndef __x86_64__
>         popl KEYP
>  #endif
> diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> index b1d90c25975a..c807b2f48ea3 100644
> --- a/arch/x86/crypto/aesni-intel_glue.c
> +++ b/arch/x86/crypto/aesni-intel_glue.c
> @@ -87,8 +87,8 @@ static inline void *aes_align_addr(void *addr)
>         return PTR_ALIGN(addr, AESNI_ALIGN);
>  }
>
> -asmlinkage int aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
> -                            unsigned int key_len);
> +asmlinkage void aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
> +                             unsigned int key_len);
>  asmlinkage void aesni_enc(const void *ctx, u8 *out, const u8 *in);
>  asmlinkage void aesni_dec(const void *ctx, u8 *out, const u8 *in);
>  asmlinkage void aesni_ecb_enc(struct crypto_aes_ctx *ctx, u8 *out,
> @@ -241,7 +241,7 @@ static int aes_set_key_common(struct crypto_aes_ctx *ctx,
>                 err = aes_expandkey(ctx, in_key, key_len);
>         else {
>                 kernel_fpu_begin();
> -               err = aesni_set_key(ctx, in_key, key_len);
> +               aesni_set_key(ctx, in_key, key_len);

This will leave 'err' uninitialized.

>                 kernel_fpu_end();
>         }
>
> --
> 2.34.1
>
>
Re: [PATCH] crypto: x86/aesni - Update aesni_set_key() to return void
Posted by Chang S. Bae 1 year, 11 months ago 
On 3/12/2024 12:46 AM, Ard Biesheuvel wrote:
> On Mon, 11 Mar 2024 at 22:48, Chang S. Bae <chang.seok.bae@intel.com> wrote:
>>
>> @@ -241,7 +241,7 @@ static int aes_set_key_common(struct crypto_aes_ctx *ctx,
>>                  err = aes_expandkey(ctx, in_key, key_len);
>>          else {
>>                  kernel_fpu_begin();
>> -               err = aesni_set_key(ctx, in_key, key_len);
>> +               aesni_set_key(ctx, in_key, key_len);
> 
> This will leave 'err' uninitialized.

Ah, right. Thanks for catching it.

Also, upon reviewing aes_expandkey(), I noticed there's no error case, 
except for the key length sanity check.

While addressing this, perhaps additional cleanup is considerable like:

@@ -233,19 +233,20 @@ static int aes_set_key_common(struct 
crypto_aes_ctx *ctx,
  {
         int err;

-       if (key_len != AES_KEYSIZE_128 && key_len != AES_KEYSIZE_192 &&
-           key_len != AES_KEYSIZE_256)
-               return -EINVAL;
+       err = aes_check_keylen(key_len);
+       if (err)
+               return err;

         if (!crypto_simd_usable())
-               err = aes_expandkey(ctx, in_key, key_len);
+               /* no error with a valid key length  */
+               aes_expandkey(ctx, in_key, key_len);
         else {
                 kernel_fpu_begin();
-               err = aesni_set_key(ctx, in_key, key_len);
+               aesni_set_key(ctx, in_key, key_len);
                 kernel_fpu_end();
         }

-       return err;
+       return 0;
  }

Thanks,
Chang
Re: [PATCH] crypto: x86/aesni - Update aesni_set_key() to return void
Posted by Ard Biesheuvel 1 year, 11 months ago 
On Tue, 12 Mar 2024 at 16:03, Chang S. Bae <chang.seok.bae@intel.com> wrote:
>
> On 3/12/2024 12:46 AM, Ard Biesheuvel wrote:
> > On Mon, 11 Mar 2024 at 22:48, Chang S. Bae <chang.seok.bae@intel.com> wrote:
> >>
> >> @@ -241,7 +241,7 @@ static int aes_set_key_common(struct crypto_aes_ctx *ctx,
> >>                  err = aes_expandkey(ctx, in_key, key_len);
> >>          else {
> >>                  kernel_fpu_begin();
> >> -               err = aesni_set_key(ctx, in_key, key_len);
> >> +               aesni_set_key(ctx, in_key, key_len);
> >
> > This will leave 'err' uninitialized.
>
> Ah, right. Thanks for catching it.
>
> Also, upon reviewing aes_expandkey(), I noticed there's no error case,
> except for the key length sanity check.
>
> While addressing this, perhaps additional cleanup is considerable like:
>
> @@ -233,19 +233,20 @@ static int aes_set_key_common(struct
> crypto_aes_ctx *ctx,
>   {
>          int err;
>
> -       if (key_len != AES_KEYSIZE_128 && key_len != AES_KEYSIZE_192 &&
> -           key_len != AES_KEYSIZE_256)
> -               return -EINVAL;
> +       err = aes_check_keylen(key_len);
> +       if (err)
> +               return err;
>
>          if (!crypto_simd_usable())
> -               err = aes_expandkey(ctx, in_key, key_len);
> +               /* no error with a valid key length  */
> +               aes_expandkey(ctx, in_key, key_len);
>          else {
>                  kernel_fpu_begin();
> -               err = aesni_set_key(ctx, in_key, key_len);
> +               aesni_set_key(ctx, in_key, key_len);
>                  kernel_fpu_end();
>          }
>
> -       return err;
> +       return 0;
>   }
>

I wonder whether we need aesni_set_key() at all.
Re: [PATCH] crypto: x86/aesni - Update aesni_set_key() to return void
Posted by Chang S. Bae 1 year, 11 months ago 
On 3/12/2024 8:18 AM, Ard Biesheuvel wrote:
> 
> I wonder whether we need aesni_set_key() at all.

The following looks to be relevant from the AES-NI whitepaper [1]:

The Relative Cost of the Key Expansion
     ...
     Some less frequent applications require frequent key scheduling. For
     example, some random number generators may rekey frequently to
     achieve forward secrecy. One extreme example is a Davies-Meyer
     hashing construction, which uses a block cipher primitive as a
     compression function, and the cipher is re-keyed for each processed
     data block.

     Although these are not the mainstream usage models of the AES
     instructions, we point out that the AESKEYGENASSIST and AESIMC
     instructions facilitate Key Expansion procedure which is lookup
     tables free, and faster than software only key expansion. In
     addition, we point out that unrolling of the key expansion code,
     which is provided in the previous sections, improves the key
     expansion performance. The AES256 case can also utilize the
     instruction AESENCLAST, for the sbox transformation, that is faster
     than using AESKEYGENASSIST.

[1] 
https://www.intel.com/content/dam/doc/white-paper/advanced-encryption-standard-new-instructions-set-paper.pdf

Thanks,
Chang
Re: [PATCH] crypto: x86/aesni - Update aesni_set_key() to return void
Posted by Eric Biggers 1 year, 11 months ago 
On Mon, Mar 11, 2024 at 02:32:32PM -0700, Chang S. Bae wrote:
> The aesni_set_key() implementation has no error case, yet its prototype
> specifies to return an error code.
> 
> Modify the function prototype to return void and adjust the related code.
> 
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: linux-crypto@vger.kernel.org
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> Previously, Eric identified a similar case in my AES-KL code [1]. Then,
> this parallel issue was realized.
> 
> [1]: https://lore.kernel.org/lkml/20230607053558.GC941@sol.localdomain/
> ---
>  arch/x86/crypto/aesni-intel_asm.S  | 5 ++---
>  arch/x86/crypto/aesni-intel_glue.c | 6 +++---
>  2 files changed, 5 insertions(+), 6 deletions(-)

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.