diff mbox series

[v4,06/30] crypto: caam/des - switch to new verification routines

Message ID 20190805170037.31330-7-ard.biesheuvel@linaro.org (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series crypto: DES/3DES cleanup | expand

Commit Message

Ard Biesheuvel Aug. 5, 2019, 5 p.m. UTC 
Tested-by: Horia Geantă <horia.geanta@nxp.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/crypto/caam/caamalg.c     | 38 +++++++-------------
 drivers/crypto/caam/caamalg_qi.c  | 13 ++-----
 drivers/crypto/caam/caamalg_qi2.c | 13 ++-----
 drivers/crypto/caam/compat.h      |  2 +-
 4 files changed, 19 insertions(+), 47 deletions(-)

Comments

Herbert Xu Aug. 15, 2019, 4:54 a.m. UTC | #1 
On Mon, Aug 05, 2019 at 08:00:13PM +0300, Ard Biesheuvel wrote:
>
> @@ -644,14 +643,8 @@ static int des3_aead_setkey(struct crypto_aead *aead, const u8 *key,
>  	if (keys.enckeylen != DES3_EDE_KEY_SIZE)
>  		goto badkey;
>  
> -	flags = crypto_aead_get_flags(aead);
> -	err = __des3_verify_key(&flags, keys.enckey);
> -	if (unlikely(err)) {
> -		crypto_aead_set_flags(aead, flags);
> -		goto out;
> -	}
> -
> -	err = aead_setkey(aead, key, keylen);
> +	err = crypto_des3_ede_verify_key(crypto_aead_tfm(aead), keys.enckey) ?:
> +	      aead_setkey(aead, key, keylen);

Please don't use crypto_aead_tfm in new code (except in core crypto
API code).

You should instead provide separate helpers that are type-specific.
So crypto_aead_des3_ede_verify_key or verify_aead_des3_key to be
more succinct.

Thanks,
Ard Biesheuvel Aug. 15, 2019, 5:01 a.m. UTC | #2 
On Thu, 15 Aug 2019 at 07:54, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Aug 05, 2019 at 08:00:13PM +0300, Ard Biesheuvel wrote:
> >
> > @@ -644,14 +643,8 @@ static int des3_aead_setkey(struct crypto_aead *aead, const u8 *key,
> >       if (keys.enckeylen != DES3_EDE_KEY_SIZE)
> >               goto badkey;
> >
> > -     flags = crypto_aead_get_flags(aead);
> > -     err = __des3_verify_key(&flags, keys.enckey);
> > -     if (unlikely(err)) {
> > -             crypto_aead_set_flags(aead, flags);
> > -             goto out;
> > -     }
> > -
> > -     err = aead_setkey(aead, key, keylen);
> > +     err = crypto_des3_ede_verify_key(crypto_aead_tfm(aead), keys.enckey) ?:
> > +           aead_setkey(aead, key, keylen);
>
> Please don't use crypto_aead_tfm in new code (except in core crypto
> API code).
>
> You should instead provide separate helpers that are type-specific.
> So crypto_aead_des3_ede_verify_key or verify_aead_des3_key to be
> more succinct.
>

OK
Ard Biesheuvel Aug. 15, 2019, 5:43 a.m. UTC | #3 
On Thu, 15 Aug 2019 at 08:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Thu, 15 Aug 2019 at 07:54, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > On Mon, Aug 05, 2019 at 08:00:13PM +0300, Ard Biesheuvel wrote:
> > >
> > > @@ -644,14 +643,8 @@ static int des3_aead_setkey(struct crypto_aead *aead, const u8 *key,
> > >       if (keys.enckeylen != DES3_EDE_KEY_SIZE)
> > >               goto badkey;
> > >
> > > -     flags = crypto_aead_get_flags(aead);
> > > -     err = __des3_verify_key(&flags, keys.enckey);
> > > -     if (unlikely(err)) {
> > > -             crypto_aead_set_flags(aead, flags);
> > > -             goto out;
> > > -     }
> > > -
> > > -     err = aead_setkey(aead, key, keylen);
> > > +     err = crypto_des3_ede_verify_key(crypto_aead_tfm(aead), keys.enckey) ?:
> > > +           aead_setkey(aead, key, keylen);
> >
> > Please don't use crypto_aead_tfm in new code (except in core crypto
> > API code).
> >
> > You should instead provide separate helpers that are type-specific.
> > So crypto_aead_des3_ede_verify_key or verify_aead_des3_key to be
> > more succinct.
> >
>
> OK

So I will end up with

static inline int verify_skcipher_des_key(struct crypto_skcipher *tfm,
  const u8 *key)
static inline int verify_skcipher_des3_key(struct crypto_skcipher *tfm,
   const u8 *key)
static inline int verify_ablkcipher_des_key(struct crypto_skcipher *tfm,
    const u8 *key)
static inline int verify_ablkcipher_des3_key(struct crypto_skcipher *tfm,
     const u8 *key)
static inline int verify_aead_des3_key(struct crypto_aead *tfm, const u8 *key,
       int keylen)
static inline int verify_aead_des_key(struct crypto_aead *tfm, const u8 *key,
      int keylen)

Is that what you had in mind?
Herbert Xu Aug. 15, 2019, 11:37 a.m. UTC | #4 
On Thu, Aug 15, 2019 at 08:43:38AM +0300, Ard Biesheuvel wrote:
>
> So I will end up with
> 
> static inline int verify_skcipher_des_key(struct crypto_skcipher *tfm,
>   const u8 *key)
> static inline int verify_skcipher_des3_key(struct crypto_skcipher *tfm,
>    const u8 *key)
> static inline int verify_ablkcipher_des_key(struct crypto_skcipher *tfm,
>     const u8 *key)
> static inline int verify_ablkcipher_des3_key(struct crypto_skcipher *tfm,
>      const u8 *key)
> static inline int verify_aead_des3_key(struct crypto_aead *tfm, const u8 *key,
>        int keylen)
> static inline int verify_aead_des_key(struct crypto_aead *tfm, const u8 *key,
>       int keylen)
> 
> Is that what you had in mind?

Yes and hopefully we will be able to get rid of ablkcipher at some
point.

As a rule we want to make the job as easy as possible for driver
authors so we should leave the burden of such trivial things with
the API.

Thanks,
diff mbox series

Patch

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 43f18253e5b6..9a9a55263b17 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -633,7 +633,6 @@  static int des3_aead_setkey(struct crypto_aead *aead, const u8 *key,
 			    unsigned int keylen)
 {
 	struct crypto_authenc_keys keys;
-	u32 flags;
 	int err;
 
 	err = crypto_authenc_extractkeys(&keys, key, keylen);
@@ -644,14 +643,8 @@  static int des3_aead_setkey(struct crypto_aead *aead, const u8 *key,
 	if (keys.enckeylen != DES3_EDE_KEY_SIZE)
 		goto badkey;
 
-	flags = crypto_aead_get_flags(aead);
-	err = __des3_verify_key(&flags, keys.enckey);
-	if (unlikely(err)) {
-		crypto_aead_set_flags(aead, flags);
-		goto out;
-	}
-
-	err = aead_setkey(aead, key, keylen);
+	err = crypto_des3_ede_verify_key(crypto_aead_tfm(aead), keys.enckey) ?:
+	      aead_setkey(aead, key, keylen);
 
 out:
 	memzero_explicit(&keys, sizeof(keys));
@@ -785,22 +778,15 @@  static int skcipher_setkey(struct crypto_skcipher *skcipher, const u8 *key,
 static int des_skcipher_setkey(struct crypto_skcipher *skcipher,
 			       const u8 *key, unsigned int keylen)
 {
-	u32 tmp[DES3_EDE_EXPKEY_WORDS];
-	struct crypto_tfm *tfm = crypto_skcipher_tfm(skcipher);
-
-	if (keylen == DES3_EDE_KEY_SIZE &&
-	    __des3_ede_setkey(tmp, &tfm->crt_flags, key, DES3_EDE_KEY_SIZE)) {
-		return -EINVAL;
-	}
-
-	if (!des_ekey(tmp, key) && (crypto_skcipher_get_flags(skcipher) &
-	    CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) {
-		crypto_skcipher_set_flags(skcipher,
-					  CRYPTO_TFM_RES_WEAK_KEY);
-		return -EINVAL;
-	}
+	return crypto_des_verify_key(crypto_skcipher_tfm(skcipher), key) ?:
+	       skcipher_setkey(skcipher, key, keylen);
+}
 
-	return skcipher_setkey(skcipher, key, keylen);
+static int des3_skcipher_setkey(struct crypto_skcipher *skcipher,
+				const u8 *key, unsigned int keylen)
+{
+	return crypto_des3_ede_verify_key(crypto_skcipher_tfm(skcipher), key) ?:
+	       skcipher_setkey(skcipher, key, keylen);
 }
 
 static int xts_skcipher_setkey(struct crypto_skcipher *skcipher, const u8 *key,
@@ -1899,7 +1885,7 @@  static struct caam_skcipher_alg driver_algs[] = {
 				.cra_driver_name = "cbc-3des-caam",
 				.cra_blocksize = DES3_EDE_BLOCK_SIZE,
 			},
-			.setkey = des_skcipher_setkey,
+			.setkey = des3_skcipher_setkey,
 			.encrypt = skcipher_encrypt,
 			.decrypt = skcipher_decrypt,
 			.min_keysize = DES3_EDE_KEY_SIZE,
@@ -2018,7 +2004,7 @@  static struct caam_skcipher_alg driver_algs[] = {
 				.cra_driver_name = "ecb-des3-caam",
 				.cra_blocksize = DES3_EDE_BLOCK_SIZE,
 			},
-			.setkey = des_skcipher_setkey,
+			.setkey = des3_skcipher_setkey,
 			.encrypt = skcipher_encrypt,
 			.decrypt = skcipher_decrypt,
 			.min_keysize = DES3_EDE_KEY_SIZE,
diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c
index 32f0f8a72067..b3868c996af8 100644
--- a/drivers/crypto/caam/caamalg_qi.c
+++ b/drivers/crypto/caam/caamalg_qi.c
@@ -296,7 +296,6 @@  static int des3_aead_setkey(struct crypto_aead *aead, const u8 *key,
 			    unsigned int keylen)
 {
 	struct crypto_authenc_keys keys;
-	u32 flags;
 	int err;
 
 	err = crypto_authenc_extractkeys(&keys, key, keylen);
@@ -307,14 +306,8 @@  static int des3_aead_setkey(struct crypto_aead *aead, const u8 *key,
 	if (keys.enckeylen != DES3_EDE_KEY_SIZE)
 		goto badkey;
 
-	flags = crypto_aead_get_flags(aead);
-	err = __des3_verify_key(&flags, keys.enckey);
-	if (unlikely(err)) {
-		crypto_aead_set_flags(aead, flags);
-		goto out;
-	}
-
-	err = aead_setkey(aead, key, keylen);
+	err = crypto_des3_ede_verify_key(crypto_aead_tfm(aead), keys.enckey) ?:
+	      aead_setkey(aead, key, keylen);
 
 out:
 	memzero_explicit(&keys, sizeof(keys));
@@ -697,7 +690,7 @@  static int skcipher_setkey(struct crypto_skcipher *skcipher, const u8 *key,
 static int des3_skcipher_setkey(struct crypto_skcipher *skcipher,
 				const u8 *key, unsigned int keylen)
 {
-	return unlikely(des3_verify_key(skcipher, key)) ?:
+	return crypto_des3_ede_verify_key(crypto_skcipher_tfm(skcipher), key) ?:
 	       skcipher_setkey(skcipher, key, keylen);
 }
 
diff --git a/drivers/crypto/caam/caamalg_qi2.c b/drivers/crypto/caam/caamalg_qi2.c
index a78a36dfa7b9..66a11ef7fd96 100644
--- a/drivers/crypto/caam/caamalg_qi2.c
+++ b/drivers/crypto/caam/caamalg_qi2.c
@@ -330,7 +330,6 @@  static int des3_aead_setkey(struct crypto_aead *aead, const u8 *key,
 			    unsigned int keylen)
 {
 	struct crypto_authenc_keys keys;
-	u32 flags;
 	int err;
 
 	err = crypto_authenc_extractkeys(&keys, key, keylen);
@@ -341,14 +340,8 @@  static int des3_aead_setkey(struct crypto_aead *aead, const u8 *key,
 	if (keys.enckeylen != DES3_EDE_KEY_SIZE)
 		goto badkey;
 
-	flags = crypto_aead_get_flags(aead);
-	err = __des3_verify_key(&flags, keys.enckey);
-	if (unlikely(err)) {
-		crypto_aead_set_flags(aead, flags);
-		goto out;
-	}
-
-	err = aead_setkey(aead, key, keylen);
+	err = crypto_des3_ede_verify_key(crypto_aead_tfm(aead), keys.enckey) ?:
+	      aead_setkey(aead, key, keylen);
 
 out:
 	memzero_explicit(&keys, sizeof(keys));
@@ -1000,7 +993,7 @@  static int skcipher_setkey(struct crypto_skcipher *skcipher, const u8 *key,
 static int des3_skcipher_setkey(struct crypto_skcipher *skcipher,
 				const u8 *key, unsigned int keylen)
 {
-	return unlikely(des3_verify_key(skcipher, key)) ?:
+	return crypto_des3_ede_verify_key(crypto_skcipher_tfm(skcipher), key) ?:
 	       skcipher_setkey(skcipher, key, keylen);
 }
 
diff --git a/drivers/crypto/caam/compat.h b/drivers/crypto/caam/compat.h
index 8639b2df0371..60e2a54c19f1 100644
--- a/drivers/crypto/caam/compat.h
+++ b/drivers/crypto/caam/compat.h
@@ -32,7 +32,7 @@ 
 #include <crypto/null.h>
 #include <crypto/aes.h>
 #include <crypto/ctr.h>
-#include <crypto/des.h>
+#include <crypto/internal/des.h>
 #include <crypto/gcm.h>
 #include <crypto/sha.h>
 #include <crypto/md5.h>