[PATCH v4 00/14] fixes for kpp and akcipher

[PATCH v4 00/14] fixes for kpp and akcipher

[Tudor Ambarus]

Hi,

These are various fixes that I made while reading kpp and akcipher
implementations.

Changes in v4:
 - assume key is already set when calling crypto_akcipher/kpp_maxsize()

v3 can be found at:
http://www.mail-archive.com/linux-crypto@vger.kernel.org/msg25312.html

Tudor Ambarus (14):
  crypto: kpp, (ec)dh - fix typos
  crypto: ecc - remove unused function arguments
  crypto: ecc - remove unnecessary casts
  crypto: ecc - don't be selfish on pubkeys
  crypto: dh - fix memleak in setkey
  crypto: kpp: maxsize() - assume key is already set
  crypto: dh - comply with crypto_kpp_maxsize()
  crypto: ecdh - comply with crypto_kpp_maxsize()
  crypto: qat - comply with crypto_kpp_maxsize()
  crypto: akcipher: maxsize() - assume key is already set
  crypto: rsa - comply with crypto_akcipher_maxsize()
  crypto: caampkc - comply with crypto_akcipher_maxsize()
  crypto: qat - comply with crypto_akcipher_maxsize()
  crypto: pkcs1pad - comply with crypto_akcipher_maxsize()

 crypto/dh.c                                   |  9 +++++---
 crypto/dh_helper.c                            |  4 ++--
 crypto/ecc.c                                  | 32 ++++++++++++---------------
 crypto/ecc.h                                  | 25 +++++++++------------
 crypto/ecdh.c                                 | 25 +++++++++------------
 crypto/ecdh_helper.c                          |  4 ++--
 crypto/rsa-pkcs1pad.c                         | 10 ++-------
 crypto/rsa.c                                  |  4 ++--
 drivers/crypto/caam/caampkc.c                 |  5 ++---
 drivers/crypto/qat/qat_common/qat_asym_algs.c |  8 +++----
 include/crypto/akcipher.h                     | 11 ++++-----
 include/crypto/dh.h                           |  4 ++--
 include/crypto/ecdh.h                         |  4 ++--
 include/crypto/kpp.h                          | 15 +++++++------
 14 files changed, 73 insertions(+), 87 deletions(-)

-- 
2.7.4

[PATCH v4 01/14] crypto: kpp, (ec)dh - fix typos

[Tudor Ambarus]

While here, add missing argument description (ndigits).

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 crypto/dh.c           | 4 ++--
 crypto/dh_helper.c    | 4 ++--
 crypto/ecc.h          | 8 +++++---
 crypto/ecdh.c         | 4 ++--
 crypto/ecdh_helper.c  | 4 ++--
 include/crypto/dh.h   | 4 ++--
 include/crypto/ecdh.h | 4 ++--
 include/crypto/kpp.h  | 4 ++--
 8 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/crypto/dh.c b/crypto/dh.c
index 87e3542..7cec0498 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -4,9 +4,9 @@
  * Authors: Salvatore Benedetto <salvatore.benedetto@intel.com>
  *
  * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public Licence
+ * modify it under the terms of the GNU General Public License
  * as published by the Free Software Foundation; either version
- * 2 of the Licence, or (at your option) any later version.
+ * 2 of the License, or (at your option) any later version.
  */
 
 #include <linux/module.h>
diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c
index 02db76b..8ba8a3f 100644
--- a/crypto/dh_helper.c
+++ b/crypto/dh_helper.c
@@ -3,9 +3,9 @@
  * Authors: Salvatore Benedetto <salvatore.benedetto@intel.com>
  *
  * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public Licence
+ * modify it under the terms of the GNU General Public License
  * as published by the Free Software Foundation; either version
- * 2 of the Licence, or (at your option) any later version.
+ * 2 of the License, or (at your option) any later version.
  */
 #include <linux/kernel.h>
 #include <linux/export.h>
diff --git a/crypto/ecc.h b/crypto/ecc.h
index 663d598..37f4385 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -34,9 +34,9 @@
  * ecc_is_key_valid() - Validate a given ECDH private key
  *
  * @curve_id:		id representing the curve to use
- * @ndigits:		curve number of digits
+ * @ndigits:		curve's number of digits
  * @private_key:	private key to be used for the given curve
- * @private_key_len:	private key len
+ * @private_key_len:	private key length
  *
  * Returns 0 if the key is acceptable, a negative value otherwise
  */
@@ -47,9 +47,10 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
  * ecdh_make_pub_key() - Compute an ECC public key
  *
  * @curve_id:		id representing the curve to use
+ * @ndigits:		curve's number of digits
  * @private_key:	pregenerated private key for the given curve
  * @private_key_len:	length of private_key
- * @public_key:		buffer for storing the public key generated
+ * @public_key:		buffer for storing the generated public key
  * @public_key_len:	length of the public_key buffer
  *
  * Returns 0 if the public key was generated successfully, a negative value
@@ -63,6 +64,7 @@ int ecdh_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
  * crypto_ecdh_shared_secret() - Compute a shared secret
  *
  * @curve_id:		id representing the curve to use
+ * @ndigits:		curve's number of digits
  * @private_key:	private key of part A
  * @private_key_len:	length of private_key
  * @public_key:		public key of counterpart B
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index 63ca337..3623307 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -4,9 +4,9 @@
  * Authors: Salvator Benedetto <salvatore.benedetto@intel.com>
  *
  * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public Licence
+ * modify it under the terms of the GNU General Public License
  * as published by the Free Software Foundation; either version
- * 2 of the Licence, or (at your option) any later version.
+ * 2 of the License, or (at your option) any later version.
  */
 
 #include <linux/module.h>
diff --git a/crypto/ecdh_helper.c b/crypto/ecdh_helper.c
index 3cd8a24..f05bea5 100644
--- a/crypto/ecdh_helper.c
+++ b/crypto/ecdh_helper.c
@@ -3,9 +3,9 @@
  * Authors: Salvatore Benedetto <salvatore.benedetto@intel.com>
  *
  * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public Licence
+ * modify it under the terms of the GNU General Public License
  * as published by the Free Software Foundation; either version
- * 2 of the Licence, or (at your option) any later version.
+ * 2 of the License, or (at your option) any later version.
  */
 #include <linux/kernel.h>
 #include <linux/export.h>
diff --git a/include/crypto/dh.h b/include/crypto/dh.h
index 6b424ad..f638998 100644
--- a/include/crypto/dh.h
+++ b/include/crypto/dh.h
@@ -73,9 +73,9 @@ int crypto_dh_encode_key(char *buf, unsigned int len, const struct dh *params);
 /**
  * crypto_dh_decode_key() - decode a private key
  * @buf:	Buffer holding a packet key that should be decoded
- * @len:	Lenth of the packet private key buffer
+ * @len:	Length of the packet private key buffer
  * @params:	Buffer allocated by the caller that is filled with the
- *		unpacket DH private key.
+ *		unpacked DH private key.
  *
  * The unpacking obtains the private key by pointing @p to the correct location
  * in @buf. Thus, both pointers refer to the same memory.
diff --git a/include/crypto/ecdh.h b/include/crypto/ecdh.h
index 03a64f6..1aff2a8 100644
--- a/include/crypto/ecdh.h
+++ b/include/crypto/ecdh.h
@@ -74,9 +74,9 @@ int crypto_ecdh_encode_key(char *buf, unsigned int len, const struct ecdh *p);
 /**
  * crypto_ecdh_decode_key() - decode a private key
  * @buf:	Buffer holding a packet key that should be decoded
- * @len:	Lenth of the packet private key buffer
+ * @len:	Length of the packet private key buffer
  * @p:		Buffer allocated by the caller that is filled with the
- *		unpacket ECDH private key.
+ *		unpacked ECDH private key.
  *
  * The unpacking obtains the private key by pointing @p to the correct location
  * in @buf. Thus, both pointers refer to the same memory.
diff --git a/include/crypto/kpp.h b/include/crypto/kpp.h
index ce8e1f7..c190825 100644
--- a/include/crypto/kpp.h
+++ b/include/crypto/kpp.h
@@ -53,7 +53,7 @@ struct crypto_kpp {
  *
  * @set_secret:		Function invokes the protocol specific function to
  *			store the secret private key along with parameters.
- *			The implementation knows how to decode thie buffer
+ *			The implementation knows how to decode the buffer
  * @generate_public_key: Function generate the public key to be sent to the
  *			counterpart. In case of error, where output is not big
  *			enough req->dst_len will be updated to the size
@@ -102,7 +102,7 @@ struct kpp_alg {
  * @mask: specifies the mask for the algorithm
  *
  * Allocate a handle for kpp algorithm. The returned struct crypto_kpp
- * is requeried for any following API invocation
+ * is required for any following API invocation
  *
  * Return: allocated handle in case of success; IS_ERR() is true in case of
  *	   an error, PTR_ERR() returns the error code.
-- 
2.7.4

[PATCH v4 02/14] crypto: ecc - remove unused function arguments

[Tudor Ambarus]

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 crypto/ecc.c  |  8 +++-----
 crypto/ecc.h  | 13 +++----------
 crypto/ecdh.c | 11 +++++------
 3 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 414c78a..69b4cc4 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -928,8 +928,7 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
 }
 
 int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
-		      const u8 *private_key, unsigned int private_key_len,
-		      u8 *public_key, unsigned int public_key_len)
+		      const u8 *private_key, u8 *public_key)
 {
 	int ret = 0;
 	struct ecc_point *pk;
@@ -967,9 +966,8 @@ int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
 }
 
 int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
-		       const u8 *private_key, unsigned int private_key_len,
-		       const u8 *public_key, unsigned int public_key_len,
-		       u8 *secret, unsigned int secret_len)
+			      const u8 *private_key, const u8 *public_key,
+			      u8 *secret)
 {
 	int ret = 0;
 	struct ecc_point *product, *pk;
diff --git a/crypto/ecc.h b/crypto/ecc.h
index 37f4385..1ca9bf7 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -49,16 +49,13 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
  * @curve_id:		id representing the curve to use
  * @ndigits:		curve's number of digits
  * @private_key:	pregenerated private key for the given curve
- * @private_key_len:	length of private_key
  * @public_key:		buffer for storing the generated public key
- * @public_key_len:	length of the public_key buffer
  *
  * Returns 0 if the public key was generated successfully, a negative value
  * if an error occurred.
  */
 int ecdh_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
-		      const u8 *private_key, unsigned int private_key_len,
-		      u8 *public_key, unsigned int public_key_len);
+		      const u8 *private_key, u8 *public_key);
 
 /**
  * crypto_ecdh_shared_secret() - Compute a shared secret
@@ -66,11 +63,8 @@ int ecdh_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
  * @curve_id:		id representing the curve to use
  * @ndigits:		curve's number of digits
  * @private_key:	private key of part A
- * @private_key_len:	length of private_key
  * @public_key:		public key of counterpart B
- * @public_key_len:	length of public_key
  * @secret:		buffer for storing the calculated shared secret
- * @secret_len:		length of the secret buffer
  *
  * Note: It is recommended that you hash the result of crypto_ecdh_shared_secret
  * before using it for symmetric encryption or HMAC.
@@ -79,7 +73,6 @@ int ecdh_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
  * if an error occurred.
  */
 int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
-		       const u8 *private_key, unsigned int private_key_len,
-		       const u8 *public_key, unsigned int public_key_len,
-		       u8 *secret, unsigned int secret_len);
+			      const u8 *private_key, const u8 *public_key,
+			      u8 *secret);
 #endif
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index 3623307..69c3951 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -81,16 +81,15 @@ static int ecdh_compute_value(struct kpp_request *req)
 			return -EINVAL;
 
 		ret = crypto_ecdh_shared_secret(ctx->curve_id, ctx->ndigits,
-					 (const u8 *)ctx->private_key, nbytes,
-					 (const u8 *)ctx->public_key, 2 * nbytes,
-					 (u8 *)ctx->shared_secret, nbytes);
+						(const u8 *)ctx->private_key,
+						(const u8 *)ctx->public_key,
+						(u8 *)ctx->shared_secret);
 
 		buf = ctx->shared_secret;
 	} else {
 		ret = ecdh_make_pub_key(ctx->curve_id, ctx->ndigits,
-					(const u8 *)ctx->private_key, nbytes,
-					(u8 *)ctx->public_key,
-					sizeof(ctx->public_key));
+					(const u8 *)ctx->private_key,
+					(u8 *)ctx->public_key);
 		buf = ctx->public_key;
 		/* Public part is a point thus it has both coordinates */
 		nbytes *= 2;
-- 
2.7.4

[PATCH v4 03/14] crypto: ecc - remove unnecessary casts

[Tudor Ambarus]

ecc software implementation works with chunks of u64 data. There were some
unnecessary casts to u8 and then back to u64 for the ecc keys. This patch
removes the unnecessary casts.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 crypto/ecc.c  | 28 +++++++++++++---------------
 crypto/ecc.h  |  8 ++++----
 crypto/ecdh.c | 11 +++++------
 3 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 69b4cc4..e3a2b8f 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -904,7 +904,7 @@ static inline void ecc_swap_digits(const u64 *in, u64 *out,
 }
 
 int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
-		     const u8 *private_key, unsigned int private_key_len)
+		     const u64 *private_key, unsigned int private_key_len)
 {
 	int nbytes;
 	const struct ecc_curve *curve = ecc_get_curve(curve_id);
@@ -917,23 +917,22 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
 	if (private_key_len != nbytes)
 		return -EINVAL;
 
-	if (vli_is_zero((const u64 *)&private_key[0], ndigits))
+	if (vli_is_zero(private_key, ndigits))
 		return -EINVAL;
 
 	/* Make sure the private key is in the range [1, n-1]. */
-	if (vli_cmp(curve->n, (const u64 *)&private_key[0], ndigits) != 1)
+	if (vli_cmp(curve->n, private_key, ndigits) != 1)
 		return -EINVAL;
 
 	return 0;
 }
 
 int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
-		      const u8 *private_key, u8 *public_key)
+		      const u64 *private_key, u64 *public_key)
 {
 	int ret = 0;
 	struct ecc_point *pk;
 	u64 priv[ndigits];
-	unsigned int nbytes;
 	const struct ecc_curve *curve = ecc_get_curve(curve_id);
 
 	if (!private_key || !curve) {
@@ -941,7 +940,7 @@ int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
 		goto out;
 	}
 
-	ecc_swap_digits((const u64 *)private_key, priv, ndigits);
+	ecc_swap_digits(private_key, priv, ndigits);
 
 	pk = ecc_alloc_point(ndigits);
 	if (!pk) {
@@ -955,9 +954,8 @@ int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
 		goto err_free_point;
 	}
 
-	nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
-	ecc_swap_digits(pk->x, (u64 *)public_key, ndigits);
-	ecc_swap_digits(pk->y, (u64 *)&public_key[nbytes], ndigits);
+	ecc_swap_digits(pk->x, public_key, ndigits);
+	ecc_swap_digits(pk->y, &public_key[ndigits], ndigits);
 
 err_free_point:
 	ecc_free_point(pk);
@@ -966,8 +964,8 @@ int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
 }
 
 int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
-			      const u8 *private_key, const u8 *public_key,
-			      u8 *secret)
+			      const u64 *private_key, const u64 *public_key,
+			      u64 *secret)
 {
 	int ret = 0;
 	struct ecc_point *product, *pk;
@@ -997,13 +995,13 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
 		goto err_alloc_product;
 	}
 
-	ecc_swap_digits((const u64 *)public_key, pk->x, ndigits);
-	ecc_swap_digits((const u64 *)&public_key[nbytes], pk->y, ndigits);
-	ecc_swap_digits((const u64 *)private_key, priv, ndigits);
+	ecc_swap_digits(public_key, pk->x, ndigits);
+	ecc_swap_digits(&public_key[ndigits], pk->y, ndigits);
+	ecc_swap_digits(private_key, priv, ndigits);
 
 	ecc_point_mult(product, pk, priv, rand_z, curve->p, ndigits);
 
-	ecc_swap_digits(product->x, (u64 *)secret, ndigits);
+	ecc_swap_digits(product->x, secret, ndigits);
 
 	if (ecc_point_is_zero(product))
 		ret = -EFAULT;
diff --git a/crypto/ecc.h b/crypto/ecc.h
index 1ca9bf7..af2ffdb 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -41,7 +41,7 @@
  * Returns 0 if the key is acceptable, a negative value otherwise
  */
 int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
-		     const u8 *private_key, unsigned int private_key_len);
+		     const u64 *private_key, unsigned int private_key_len);
 
 /**
  * ecdh_make_pub_key() - Compute an ECC public key
@@ -55,7 +55,7 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
  * if an error occurred.
  */
 int ecdh_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
-		      const u8 *private_key, u8 *public_key);
+		      const u64 *private_key, u64 *public_key);
 
 /**
  * crypto_ecdh_shared_secret() - Compute a shared secret
@@ -73,6 +73,6 @@ int ecdh_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
  * if an error occurred.
  */
 int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
-			      const u8 *private_key, const u8 *public_key,
-			      u8 *secret);
+			      const u64 *private_key, const u64 *public_key,
+			      u64 *secret);
 #endif
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index 69c3951..c1f0163 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -56,7 +56,7 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
 	ctx->ndigits = ndigits;
 
 	if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
-			     (const u8 *)params.key, params.key_size) < 0)
+			     (const u64 *)params.key, params.key_size) < 0)
 		return -EINVAL;
 
 	memcpy(ctx->private_key, params.key, params.key_size);
@@ -81,15 +81,14 @@ static int ecdh_compute_value(struct kpp_request *req)
 			return -EINVAL;
 
 		ret = crypto_ecdh_shared_secret(ctx->curve_id, ctx->ndigits,
-						(const u8 *)ctx->private_key,
-						(const u8 *)ctx->public_key,
-						(u8 *)ctx->shared_secret);
+						ctx->private_key,
+						ctx->public_key,
+						ctx->shared_secret);
 
 		buf = ctx->shared_secret;
 	} else {
 		ret = ecdh_make_pub_key(ctx->curve_id, ctx->ndigits,
-					(const u8 *)ctx->private_key,
-					(u8 *)ctx->public_key);
+					ctx->private_key, ctx->public_key);
 		buf = ctx->public_key;
 		/* Public part is a point thus it has both coordinates */
 		nbytes *= 2;
-- 
2.7.4

[PATCH v4 04/14] crypto: ecc - don't be selfish on pubkeys

[Tudor Ambarus]

Rename ecdh_make_pub_key() to ecc_make_pub_key().
This function might as well be used by ecdsa.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 crypto/ecc.c  | 4 ++--
 crypto/ecc.h  | 4 ++--
 crypto/ecdh.c | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index e3a2b8f..6c33c43 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -927,8 +927,8 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
 	return 0;
 }
 
-int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
-		      const u64 *private_key, u64 *public_key)
+int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
+		     const u64 *private_key, u64 *public_key)
 {
 	int ret = 0;
 	struct ecc_point *pk;
diff --git a/crypto/ecc.h b/crypto/ecc.h
index af2ffdb..673c834 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -54,8 +54,8 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
  * Returns 0 if the public key was generated successfully, a negative value
  * if an error occurred.
  */
-int ecdh_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
-		      const u64 *private_key, u64 *public_key);
+int ecc_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
+		     const u64 *private_key, u64 *public_key);
 
 /**
  * crypto_ecdh_shared_secret() - Compute a shared secret
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index c1f0163..ed1464a 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -87,8 +87,8 @@ static int ecdh_compute_value(struct kpp_request *req)
 
 		buf = ctx->shared_secret;
 	} else {
-		ret = ecdh_make_pub_key(ctx->curve_id, ctx->ndigits,
-					ctx->private_key, ctx->public_key);
+		ret = ecc_make_pub_key(ctx->curve_id, ctx->ndigits,
+				       ctx->private_key, ctx->public_key);
 		buf = ctx->public_key;
 		/* Public part is a point thus it has both coordinates */
 		nbytes *= 2;
-- 
2.7.4

Re: [PATCH v4 04/14] crypto: ecc - don't be selfish on pubkeys

[Horia Geantă]

On 5/25/2017 10:18 AM, Tudor Ambarus wrote:
> Rename ecdh_make_pub_key() to ecc_make_pub_key().
> This function might as well be used by ecdsa.

Where exactly is ecdsa used in the kernel?
https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg23235.html

Thanks,
Horia

Re: [PATCH v4 04/14] crypto: ecc - don't be selfish on pubkeys

[Tudor Ambarus]

Hi, Horia,

On 28.05.2017 20:22, Horia Geantă wrote:
> On 5/25/2017 10:18 AM, Tudor Ambarus wrote:
>> Rename ecdh_make_pub_key() to ecc_make_pub_key().
>> This function might as well be used by ecdsa.
> 
> Where exactly is ecdsa used in the kernel?
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg23235.html

Nowhere. Maybe I should have dropped the reference to ecdsa.
ecdh_make_pub_key() is not dh specific and the reference to dh is wrong.

Thanks for the review,
ta

[PATCH v5 04/14] crypto: ecc - rename ecdh_make_pub_key()

[Tudor Ambarus]

Rename ecdh_make_pub_key() to ecc_make_pub_key().
ecdh_make_pub_key() is not dh specific and the reference
to dh is wrong.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
v5 updates the description of the function by replacing
"ecdh_make_pub_key()" with "ecc_make_pub_key()". I also
updated the commit message.

Herbert, do I need to resend a new series for this change or
having the updated patch like this is ok for you?

 crypto/ecc.c  | 4 ++--
 crypto/ecc.h  | 6 +++---
 crypto/ecdh.c | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index e3a2b8f..6c33c43 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -927,8 +927,8 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
 	return 0;
 }
 
-int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
-		      const u64 *private_key, u64 *public_key)
+int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
+		     const u64 *private_key, u64 *public_key)
 {
 	int ret = 0;
 	struct ecc_point *pk;
diff --git a/crypto/ecc.h b/crypto/ecc.h
index af2ffdb..e13fe88 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -44,7 +44,7 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
 		     const u64 *private_key, unsigned int private_key_len);
 
 /**
- * ecdh_make_pub_key() - Compute an ECC public key
+ * ecc_make_pub_key() - Compute an ECC public key
  *
  * @curve_id:		id representing the curve to use
  * @ndigits:		curve's number of digits
@@ -54,8 +54,8 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits,
  * Returns 0 if the public key was generated successfully, a negative value
  * if an error occurred.
  */
-int ecdh_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
-		      const u64 *private_key, u64 *public_key);
+int ecc_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
+		     const u64 *private_key, u64 *public_key);
 
 /**
  * crypto_ecdh_shared_secret() - Compute a shared secret
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index c1f0163..ed1464a 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -87,8 +87,8 @@ static int ecdh_compute_value(struct kpp_request *req)
 
 		buf = ctx->shared_secret;
 	} else {
-		ret = ecdh_make_pub_key(ctx->curve_id, ctx->ndigits,
-					ctx->private_key, ctx->public_key);
+		ret = ecc_make_pub_key(ctx->curve_id, ctx->ndigits,
+				       ctx->private_key, ctx->public_key);
 		buf = ctx->public_key;
 		/* Public part is a point thus it has both coordinates */
 		nbytes *= 2;
-- 
2.7.4

[PATCH v4 05/14] crypto: dh - fix memleak in setkey

[Tudor Ambarus]

setkey can be called multiple times during the existence
of the transformation object. In case of multiple setkey calls,
the old key was not freed and we leaked memory.
Free the old MPI key if any.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 crypto/dh.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/crypto/dh.c b/crypto/dh.c
index 7cec0498..e151f12 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -85,6 +85,9 @@ static int dh_set_secret(struct crypto_kpp *tfm, const void *buf,
 	struct dh_ctx *ctx = dh_get_ctx(tfm);
 	struct dh params;
 
+	/* Free the old MPI key if any */
+	dh_free_ctx(ctx);
+
 	if (crypto_dh_decode_key(buf, len, &params) < 0)
 		return -EINVAL;
 
-- 
2.7.4

[PATCH v4 06/14] crypto: kpp: maxsize() - assume key is already set

[Tudor Ambarus]

As of now, crypto_kpp_maxsize() can not be reached without successfully
setting the key for the transformation. kpp algorithm implementations
check if the key was set and then return the output buffer size
required for the given key.

Change the return type to unsigned int and always assume that this
function is called after a successful setkey of the transformation.
kpp algorithm implementations will remove the check if key is not NULL
and directly return the max size.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 include/crypto/kpp.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/crypto/kpp.h b/include/crypto/kpp.h
index c190825..2133d17 100644
--- a/include/crypto/kpp.h
+++ b/include/crypto/kpp.h
@@ -79,7 +79,7 @@ struct kpp_alg {
 	int (*generate_public_key)(struct kpp_request *req);
 	int (*compute_shared_secret)(struct kpp_request *req);
 
-	int (*max_size)(struct crypto_kpp *tfm);
+	unsigned int (*max_size)(struct crypto_kpp *tfm);
 
 	int (*init)(struct crypto_kpp *tfm);
 	void (*exit)(struct crypto_kpp *tfm);
@@ -323,13 +323,14 @@ static inline int crypto_kpp_compute_shared_secret(struct kpp_request *req)
 /**
  * crypto_kpp_maxsize() - Get len for output buffer
  *
- * Function returns the output buffer size required
+ * Function returns the output buffer size required for a given key.
+ * Function assumes that the key is already set in the transformation. If this
+ * function is called without a setkey or with a failed setkey, you will end up
+ * in a NULL dereference.
  *
  * @tfm:	KPP tfm handle allocated with crypto_alloc_kpp()
- *
- * Return: minimum len for output buffer or error code if key hasn't been set
  */
-static inline int crypto_kpp_maxsize(struct crypto_kpp *tfm)
+static inline unsigned int crypto_kpp_maxsize(struct crypto_kpp *tfm)
 {
 	struct kpp_alg *alg = crypto_kpp_alg(tfm);
 
-- 
2.7.4

[PATCH v4 07/14] crypto: dh - comply with crypto_kpp_maxsize()

[Tudor Ambarus]

crypto_kpp_maxsize() asks for the output buffer size without
caring for errors. It allways assume that will be called after
a valid setkey. Comply with it and return what he wants.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 crypto/dh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/dh.c b/crypto/dh.c
index e151f12..b1032a5 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -147,7 +147,7 @@ static int dh_compute_value(struct kpp_request *req)
 	return ret;
 }
 
-static int dh_max_size(struct crypto_kpp *tfm)
+static unsigned int dh_max_size(struct crypto_kpp *tfm)
 {
 	struct dh_ctx *ctx = dh_get_ctx(tfm);
 
-- 
2.7.4

[PATCH v4 08/14] crypto: ecdh - comply with crypto_kpp_maxsize()

[Tudor Ambarus]

crypto_kpp_maxsize() asks for the output buffer size without
caring for errors. It allways assume that will be called after
a valid setkey. Comply with it and return what he wants.

nbytes has no sense now, remove it and directly return the maxsize.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 crypto/ecdh.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index ed1464a..4aa0b0c 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -104,13 +104,12 @@ static int ecdh_compute_value(struct kpp_request *req)
 	return ret;
 }
 
-static int ecdh_max_size(struct crypto_kpp *tfm)
+static unsigned int ecdh_max_size(struct crypto_kpp *tfm)
 {
 	struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
-	int nbytes = ctx->ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
 
-	/* Public key is made of two coordinates */
-	return 2 * nbytes;
+	/* Public key is made of two coordinates, add one to the left shift */
+	return ctx->ndigits << (ECC_DIGITS_TO_BYTES_SHIFT + 1);
 }
 
 static void no_exit_tfm(struct crypto_kpp *tfm)
-- 
2.7.4

[PATCH v4 09/14] crypto: qat - comply with crypto_kpp_maxsize()

[Tudor Ambarus]

crypto_kpp_maxsize() asks for the output buffer size without
caring for errors. It allways assume that will be called after
a valid setkey. Comply with it and return what he wants.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/crypto/qat/qat_common/qat_asym_algs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c
index 2aab80b..1d882a7 100644
--- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
@@ -521,11 +521,11 @@ static int qat_dh_set_secret(struct crypto_kpp *tfm, const void *buf,
 	return 0;
 }
 
-static int qat_dh_max_size(struct crypto_kpp *tfm)
+static unsigned int qat_dh_max_size(struct crypto_kpp *tfm)
 {
 	struct qat_dh_ctx *ctx = kpp_tfm_ctx(tfm);
 
-	return ctx->p ? ctx->p_size : -EINVAL;
+	return ctx->p_size;
 }
 
 static int qat_dh_init_tfm(struct crypto_kpp *tfm)
-- 
2.7.4

[PATCH v4 10/14] crypto: akcipher: maxsize() - assume key is already set

[Tudor Ambarus]

As of now, crypto_akcipher_maxsize() can not be reached without
successfully setting the key for the transformation. akcipher
algorithm implementations check if the key was set and then return
the output buffer size required for the given key.

Change the return type to unsigned int and always assume that this
function is called after a successful setkey of the transformation.
akcipher algorithm implementations will remove the check if key is not NULL
and directly return the max size.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 include/crypto/akcipher.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
index c37cc59..b5e11de 100644
--- a/include/crypto/akcipher.h
+++ b/include/crypto/akcipher.h
@@ -98,7 +98,7 @@ struct akcipher_alg {
 			   unsigned int keylen);
 	int (*set_priv_key)(struct crypto_akcipher *tfm, const void *key,
 			    unsigned int keylen);
-	int (*max_size)(struct crypto_akcipher *tfm);
+	unsigned int (*max_size)(struct crypto_akcipher *tfm);
 	int (*init)(struct crypto_akcipher *tfm);
 	void (*exit)(struct crypto_akcipher *tfm);
 
@@ -257,13 +257,14 @@ static inline void akcipher_request_set_crypt(struct akcipher_request *req,
 /**
  * crypto_akcipher_maxsize() - Get len for output buffer
  *
- * Function returns the dest buffer size required for a given key
+ * Function returns the dest buffer size required for a given key.
+ * Function assumes that the key is already set in the transformation. If this
+ * function is called without a setkey or with a failed setkey, you will end up
+ * in a NULL dereference.
  *
  * @tfm:	AKCIPHER tfm handle allocated with crypto_alloc_akcipher()
- *
- * Return: minimum len for output buffer or error code in key hasn't been set
  */
-static inline int crypto_akcipher_maxsize(struct crypto_akcipher *tfm)
+static inline unsigned int crypto_akcipher_maxsize(struct crypto_akcipher *tfm)
 {
 	struct akcipher_alg *alg = crypto_akcipher_alg(tfm);
 
-- 
2.7.4

[PATCH v4 11/14] crypto: rsa - comply with crypto_akcipher_maxsize()

[Tudor Ambarus]

crypto_akcipher_maxsize() asks for the output buffer size without
caring for errors. It allways assume that will be called after
a valid setkey. Comply with it and return what he wants.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 crypto/rsa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/rsa.c b/crypto/rsa.c
index 4c280b6..b067f3a 100644
--- a/crypto/rsa.c
+++ b/crypto/rsa.c
@@ -337,11 +337,11 @@ static int rsa_set_priv_key(struct crypto_akcipher *tfm, const void *key,
 	return -ENOMEM;
 }
 
-static int rsa_max_size(struct crypto_akcipher *tfm)
+static unsigned int rsa_max_size(struct crypto_akcipher *tfm)
 {
 	struct rsa_mpi_key *pkey = akcipher_tfm_ctx(tfm);
 
-	return pkey->n ? mpi_get_size(pkey->n) : -EINVAL;
+	return mpi_get_size(pkey->n);
 }
 
 static void rsa_exit_tfm(struct crypto_akcipher *tfm)
-- 
2.7.4

[PATCH v4 12/14] crypto: caampkc - comply with crypto_akcipher_maxsize()

[Tudor Ambarus]

crypto_akcipher_maxsize() asks for the output buffer size without
caring for errors. It allways assume that will be called after
a valid setkey. Comply with it and return what he wants.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/crypto/caam/caampkc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
index 57f399c..9c508ba 100644
--- a/drivers/crypto/caam/caampkc.c
+++ b/drivers/crypto/caam/caampkc.c
@@ -911,12 +911,11 @@ static int caam_rsa_set_priv_key(struct crypto_akcipher *tfm, const void *key,
 	return -ENOMEM;
 }
 
-static int caam_rsa_max_size(struct crypto_akcipher *tfm)
+static unsigned int caam_rsa_max_size(struct crypto_akcipher *tfm)
 {
 	struct caam_rsa_ctx *ctx = akcipher_tfm_ctx(tfm);
-	struct caam_rsa_key *key = &ctx->key;
 
-	return (key->n) ? key->n_sz : -EINVAL;
+	return ctx->key.n_sz;
 }
 
 /* Per session pkc's driver context creation function */
-- 
2.7.4

[PATCH v4 13/14] crypto: qat - comply with crypto_akcipher_maxsize()

[Tudor Ambarus]

crypto_akcipher_maxsize() asks for the output buffer size without
caring for errors. It allways assume that will be called after
a valid setkey. Comply with it and return what he wants.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/crypto/qat/qat_common/qat_asym_algs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c
index 1d882a7..6f5dd68 100644
--- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
@@ -1256,11 +1256,11 @@ static int qat_rsa_setprivkey(struct crypto_akcipher *tfm, const void *key,
 	return qat_rsa_setkey(tfm, key, keylen, true);
 }
 
-static int qat_rsa_max_size(struct crypto_akcipher *tfm)
+static unsigned int qat_rsa_max_size(struct crypto_akcipher *tfm)
 {
 	struct qat_rsa_ctx *ctx = akcipher_tfm_ctx(tfm);
 
-	return (ctx->n) ? ctx->key_sz : -EINVAL;
+	return ctx->key_sz;
 }
 
 static int qat_rsa_init_tfm(struct crypto_akcipher *tfm)
-- 
2.7.4

[PATCH v4 14/14] crypto: pkcs1pad - comply with crypto_akcipher_maxsize()

[Tudor Ambarus]

crypto_akcipher_maxsize() asks for the output buffer size without
caring for errors. It allways assume that will be called after
a valid setkey. Comply with it and return what he wants.

crypto_akcipher_maxsize() now returns an unsigned int.
Remove the unnecessary check.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 crypto/rsa-pkcs1pad.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index 8baab43..044bcfa 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -120,9 +120,6 @@ static int pkcs1pad_set_pub_key(struct crypto_akcipher *tfm, const void *key,
 
 	/* Find out new modulus size from rsa implementation */
 	err = crypto_akcipher_maxsize(ctx->child);
-	if (err < 0)
-		return err;
-
 	if (err > PAGE_SIZE)
 		return -ENOTSUPP;
 
@@ -144,9 +141,6 @@ static int pkcs1pad_set_priv_key(struct crypto_akcipher *tfm, const void *key,
 
 	/* Find out new modulus size from rsa implementation */
 	err = crypto_akcipher_maxsize(ctx->child);
-	if (err < 0)
-		return err;
-
 	if (err > PAGE_SIZE)
 		return -ENOTSUPP;
 
@@ -154,7 +148,7 @@ static int pkcs1pad_set_priv_key(struct crypto_akcipher *tfm, const void *key,
 	return 0;
 }
 
-static int pkcs1pad_get_max_size(struct crypto_akcipher *tfm)
+static unsigned int pkcs1pad_get_max_size(struct crypto_akcipher *tfm)
 {
 	struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm);
 
@@ -164,7 +158,7 @@ static int pkcs1pad_get_max_size(struct crypto_akcipher *tfm)
 	 * decrypt/verify.
 	 */
 
-	return ctx->key_size ?: -EINVAL;
+	return ctx->key_size;
 }
 
 static void pkcs1pad_sg_set_buf(struct scatterlist *sg, void *buf, size_t len,
-- 
2.7.4

Re: [PATCH v4 00/14] fixes for kpp and akcipher

[Herbert Xu]

On Thu, May 25, 2017 at 10:18:02AM +0300, Tudor Ambarus wrote:
> Hi,
> 
> These are various fixes that I made while reading kpp and akcipher
> implementations.
> 
> Changes in v4:
>  - assume key is already set when calling crypto_akcipher/kpp_maxsize()

All applied, with patch 4/14 taken from v5.  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