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

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

[Chang S. Bae]

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

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

[Eric Biggers]

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

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

[Ard Biesheuvel]

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

[Chang S. Bae]

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

[Ard Biesheuvel]

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

[Chang S. Bae]

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

[kernel test robot]

Hi Chang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.9]
[cannot apply to herbert-cryptodev-2.6/master herbert-crypto-2.6/master linus/master next-20240523]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chang-S-Bae/crypto-x86-aesni-Update-aesni_set_key-to-return-void/20240523-071024
base:   v6.9
patch link:    https://lore.kernel.org/r/20240311213232.128240-1-chang.seok.bae%40intel.com
patch subject: [PATCH] crypto: x86/aesni - Update aesni_set_key() to return void
config: i386-buildonly-randconfig-003-20240523 (https://download.01.org/0day-ci/archive/20240523/202405231053.XErdDIz3-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240523/202405231053.XErdDIz3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405231053.XErdDIz3-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> arch/x86/crypto/aesni-intel_glue.c:240:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     240 |         if (!crypto_simd_usable())
         |             ^~~~~~~~~~~~~~~~~~~~~
   arch/x86/crypto/aesni-intel_glue.c:248:9: note: uninitialized use occurs here
     248 |         return err;
         |                ^~~
   arch/x86/crypto/aesni-intel_glue.c:240:2: note: remove the 'if' if its condition is always true
     240 |         if (!crypto_simd_usable())
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
     241 |                 err = aes_expandkey(ctx, in_key, key_len);
         |                                                          ~
     242 |         else {
         |         ~~~~~~
     243 |                 kernel_fpu_begin();
         |                 ~~~~~~~~~~~~~~~~~~~
     244 |                 aesni_set_key(ctx, in_key, key_len);
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     245 |                 kernel_fpu_end();
         |                 ~~~~~~~~~~~~~~~~~
     246 |         }
         |         ~
   arch/x86/crypto/aesni-intel_glue.c:234:9: note: initialize the variable 'err' to silence this warning
     234 |         int err;
         |                ^
         |                 = 0
   1 warning generated.


vim +240 arch/x86/crypto/aesni-intel_glue.c

e12a68b3c6ac2c Chang S. Bae   2023-09-28  230  
28b776098379fb Eric Biggers   2023-07-14  231  static int aes_set_key_common(struct crypto_aes_ctx *ctx,
54b6a1bd5364ac Huang Ying     2009-01-18  232  			      const u8 *in_key, unsigned int key_len)
54b6a1bd5364ac Huang Ying     2009-01-18  233  {
54b6a1bd5364ac Huang Ying     2009-01-18  234  	int err;
54b6a1bd5364ac Huang Ying     2009-01-18  235  
54b6a1bd5364ac Huang Ying     2009-01-18  236  	if (key_len != AES_KEYSIZE_128 && key_len != AES_KEYSIZE_192 &&
674f368a952c48 Eric Biggers   2019-12-30  237  	    key_len != AES_KEYSIZE_256)
54b6a1bd5364ac Huang Ying     2009-01-18  238  		return -EINVAL;
54b6a1bd5364ac Huang Ying     2009-01-18  239  
f2abe0d72b2167 Eric Biggers   2019-03-12 @240  	if (!crypto_simd_usable())
2c53fd11f76246 Ard Biesheuvel 2019-07-02  241  		err = aes_expandkey(ctx, in_key, key_len);
54b6a1bd5364ac Huang Ying     2009-01-18  242  	else {
54b6a1bd5364ac Huang Ying     2009-01-18  243  		kernel_fpu_begin();
36ac212f978e58 Chang S. Bae   2024-03-11  244  		aesni_set_key(ctx, in_key, key_len);
54b6a1bd5364ac Huang Ying     2009-01-18  245  		kernel_fpu_end();
54b6a1bd5364ac Huang Ying     2009-01-18  246  	}
54b6a1bd5364ac Huang Ying     2009-01-18  247  
54b6a1bd5364ac Huang Ying     2009-01-18  248  	return err;
54b6a1bd5364ac Huang Ying     2009-01-18  249  }
54b6a1bd5364ac Huang Ying     2009-01-18  250  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240311213232.128240-1-chang.seok.bae@intel.com>
References: <20240311213232.128240-1-chang.seok.bae@intel.com>
TO: "Chang S. Bae" <chang.seok.bae@intel.com>

Hi Chang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.9]
[cannot apply to herbert-cryptodev-2.6/master herbert-crypto-2.6/master linus/master next-20240523]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chang-S-Bae/crypto-x86-aesni-Update-aesni_set_key-to-return-void/20240523-071024
base:   v6.9
patch link:    https://lore.kernel.org/r/20240311213232.128240-1-chang.seok.bae%40intel.com
patch subject: [PATCH] crypto: x86/aesni - Update aesni_set_key() to return void
:::::: branch date: 31 hours ago
:::::: commit date: 31 hours ago
config: x86_64-randconfig-161-20240524 (https://download.01.org/0day-ci/archive/20240524/202405241440.ksMTxyPy-lkp@intel.com/config)
compiler: gcc-11 (Ubuntu 11.4.0-4ubuntu1) 11.4.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202405241440.ksMTxyPy-lkp@intel.com/

smatch warnings:
arch/x86/crypto/aesni-intel_glue.c:248 aes_set_key_common() error: uninitialized symbol 'err'.

vim +/err +248 arch/x86/crypto/aesni-intel_glue.c

e12a68b3c6ac2c Chang S. Bae   2023-09-28  230  
28b776098379fb Eric Biggers   2023-07-14  231  static int aes_set_key_common(struct crypto_aes_ctx *ctx,
54b6a1bd5364ac Huang Ying     2009-01-18  232  			      const u8 *in_key, unsigned int key_len)
54b6a1bd5364ac Huang Ying     2009-01-18  233  {
54b6a1bd5364ac Huang Ying     2009-01-18  234  	int err;
54b6a1bd5364ac Huang Ying     2009-01-18  235  
54b6a1bd5364ac Huang Ying     2009-01-18  236  	if (key_len != AES_KEYSIZE_128 && key_len != AES_KEYSIZE_192 &&
674f368a952c48 Eric Biggers   2019-12-30  237  	    key_len != AES_KEYSIZE_256)
54b6a1bd5364ac Huang Ying     2009-01-18  238  		return -EINVAL;
54b6a1bd5364ac Huang Ying     2009-01-18  239  
f2abe0d72b2167 Eric Biggers   2019-03-12  240  	if (!crypto_simd_usable())
2c53fd11f76246 Ard Biesheuvel 2019-07-02  241  		err = aes_expandkey(ctx, in_key, key_len);
54b6a1bd5364ac Huang Ying     2009-01-18  242  	else {
54b6a1bd5364ac Huang Ying     2009-01-18  243  		kernel_fpu_begin();
36ac212f978e58 Chang S. Bae   2024-03-11  244  		aesni_set_key(ctx, in_key, key_len);
54b6a1bd5364ac Huang Ying     2009-01-18  245  		kernel_fpu_end();
54b6a1bd5364ac Huang Ying     2009-01-18  246  	}
54b6a1bd5364ac Huang Ying     2009-01-18  247  
54b6a1bd5364ac Huang Ying     2009-01-18 @248  	return err;
54b6a1bd5364ac Huang Ying     2009-01-18  249  }
54b6a1bd5364ac Huang Ying     2009-01-18  250  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki