* [v2 RFC PATCH 1/2] crypto: ecdh: fix concurrency on ecdh_ctx
2017-06-28 13:55 [v2 RFC PATCH 0/2] crypto: ecdh, ecc fixes Tudor Ambarus
@ 2017-06-28 13:55 ` Tudor Ambarus
2017-06-28 13:55 ` [v2 RFC PATCH 2/2] crypto: ecc: use caller's GFP flags Tudor Ambarus
1 sibling, 0 replies; 3+ messages in thread
From: Tudor Ambarus @ 2017-06-28 13:55 UTC (permalink / raw)
To: herbert; +Cc: davem, linux-crypto, nicolas.ferre, Tudor Ambarus
From: Tudor Ambarus <tudor.ambarus@microchip.com>
ecdh_ctx contained static allocated data for the shared secret,
for the public and private key.
When talking about shared secret and public key, they were
doomed to concurrency issues because they could be shared by
multiple crypto requests. The requests were generating specific
data to the same zone of memory without any protection.
The private key was subject to concurrency problems because
multiple setkey calls could fight to memcpy to the same zone
of memory.
Shared secret and public key concurrency is fixed by allocating
memory on heap for each request. In the end, the shared secret
and public key are computed for each request, there is no reason
to use shared memory.
Private key concurrency is fixed by allocating memory on heap
for each setkey call, by memcopying the parsed/generated private
key to the heap and by making the private key pointer from
ecdh_ctx to point to the newly allocated data.
On all systems running Linux, loads from and stores to pointers
are atomic, that is, if a store to a pointer occurs at the same
time as a load from that same pointer, the load will return either
the initial value or the value stored, never some bitwise mashup
of the two.
With this, the private key will always point to a valid key,
but to what setkey call it belongs, is the responsibility of the
caller, as it is now in all crypto framework.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
crypto/ecc.h | 2 --
crypto/ecdh.c | 96 +++++++++++++++++++++++++++++++++++++++++------------------
2 files changed, 67 insertions(+), 31 deletions(-)
diff --git a/crypto/ecc.h b/crypto/ecc.h
index e4fd449..b35855b 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -26,8 +26,6 @@
#ifndef _CRYPTO_ECC_H
#define _CRYPTO_ECC_H
-#define ECC_MAX_DIGITS 4 /* 256 */
-
#define ECC_DIGITS_TO_BYTES_SHIFT 3
/**
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index 61c7708..e258ab3 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -19,9 +19,7 @@
struct ecdh_ctx {
unsigned int curve_id;
unsigned int ndigits;
- u64 private_key[ECC_MAX_DIGITS];
- u64 public_key[2 * ECC_MAX_DIGITS];
- u64 shared_secret[ECC_MAX_DIGITS];
+ const u64 *private_key;
};
static inline struct ecdh_ctx *ecdh_get_ctx(struct crypto_kpp *tfm)
@@ -42,8 +40,13 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
unsigned int len)
{
struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
+ u64 *private_key;
struct ecdh params;
unsigned int ndigits;
+ int ret;
+
+ /* clear the old private key, if any */
+ kzfree(ctx->private_key);
if (crypto_ecdh_decode_key(buf, len, ¶ms) < 0)
return -EINVAL;
@@ -52,59 +55,92 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
if (!ndigits)
return -EINVAL;
+ private_key = kmalloc(ndigits << ECC_DIGITS_TO_BYTES_SHIFT, GFP_KERNEL);
+ if (!private_key)
+ return -ENOMEM;
+
ctx->curve_id = params.curve_id;
ctx->ndigits = ndigits;
- if (!params.key || !params.key_size)
- return ecc_gen_privkey(ctx->curve_id, ctx->ndigits,
- ctx->private_key);
+ if (!params.key || !params.key_size) {
+ ret = ecc_gen_privkey(ctx->curve_id, ctx->ndigits, private_key);
+ if (ret)
+ goto err;
+ ctx->private_key = private_key;
+ return ret;
+ }
- if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
- (const u64 *)params.key, params.key_size) < 0)
- return -EINVAL;
+ ret = ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
+ (const u64 *)params.key, params.key_size);
+ if (ret < 0)
+ goto err;
- memcpy(ctx->private_key, params.key, params.key_size);
+ memcpy(private_key, params.key, params.key_size);
+ ctx->private_key = private_key;
return 0;
+err:
+ kzfree(private_key);
+ return ret;
}
static int ecdh_compute_value(struct kpp_request *req)
{
- int ret = 0;
struct crypto_kpp *tfm = crypto_kpp_reqtfm(req);
struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
- size_t copied, nbytes;
+ u64 *public_key;
+ u64 *shared_secret = NULL;
void *buf;
+ size_t copied, nbytes, public_key_sz;
+ gfp_t gfp;
+ int ret = -ENOMEM;
nbytes = ctx->ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
+ /* Public part is a point thus it has both coordinates */
+ public_key_sz = 2 * nbytes;
+
+ gfp = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ? GFP_KERNEL :
+ GFP_ATOMIC;
+ public_key = kmalloc(public_key_sz, gfp);
+ if (!public_key)
+ return -ENOMEM;
if (req->src) {
- copied = sg_copy_to_buffer(req->src, 1, ctx->public_key,
- 2 * nbytes);
- if (copied != 2 * nbytes)
- return -EINVAL;
+ shared_secret = kmalloc(nbytes, gfp);
+ if (!shared_secret)
+ goto free_pubkey;
+
+ copied = sg_copy_to_buffer(req->src, 1, public_key,
+ public_key_sz);
+ if (copied != public_key_sz) {
+ ret = -EINVAL;
+ goto free_all;
+ }
ret = crypto_ecdh_shared_secret(ctx->curve_id, ctx->ndigits,
- ctx->private_key,
- ctx->public_key,
- ctx->shared_secret);
+ ctx->private_key, public_key,
+ shared_secret);
- buf = ctx->shared_secret;
+ buf = shared_secret;
} else {
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;
+ ctx->private_key, public_key);
+ buf = public_key;
+ nbytes = public_key_sz;
}
if (ret < 0)
- return ret;
+ goto free_all;
copied = sg_copy_from_buffer(req->dst, 1, buf, nbytes);
if (copied != nbytes)
- return -EINVAL;
+ ret = -EINVAL;
+ /* fall through */
+free_all:
+ kzfree(shared_secret);
+free_pubkey:
+ kfree(public_key);
return ret;
}
@@ -116,9 +152,11 @@ static unsigned int ecdh_max_size(struct crypto_kpp *tfm)
return ctx->ndigits << (ECC_DIGITS_TO_BYTES_SHIFT + 1);
}
-static void no_exit_tfm(struct crypto_kpp *tfm)
+static void ecdh_exit_tfm(struct crypto_kpp *tfm)
{
- return;
+ struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
+
+ kzfree(ctx->private_key);
}
static struct kpp_alg ecdh = {
@@ -126,7 +164,7 @@ static struct kpp_alg ecdh = {
.generate_public_key = ecdh_compute_value,
.compute_shared_secret = ecdh_compute_value,
.max_size = ecdh_max_size,
- .exit = no_exit_tfm,
+ .exit = ecdh_exit_tfm,
.base = {
.cra_name = "ecdh",
.cra_driver_name = "ecdh-generic",
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* [v2 RFC PATCH 2/2] crypto: ecc: use caller's GFP flags
2017-06-28 13:55 [v2 RFC PATCH 0/2] crypto: ecdh, ecc fixes Tudor Ambarus
2017-06-28 13:55 ` [v2 RFC PATCH 1/2] crypto: ecdh: fix concurrency on ecdh_ctx Tudor Ambarus
@ 2017-06-28 13:55 ` Tudor Ambarus
1 sibling, 0 replies; 3+ messages in thread
From: Tudor Ambarus @ 2017-06-28 13:55 UTC (permalink / raw)
To: herbert; +Cc: davem, linux-crypto, nicolas.ferre, Tudor Ambarus
Using GFP_KERNEL when allocating data and implicitly
assuming that we can sleep was wrong because the caller
could be in atomic context. Let the caller decide whether
sleeping is possible or not.
The caller (ecdh) was updated in the same patch in order
to not affect bissectability.
Signed-off-by: Tudor Ambarus <tudor.ambarus@gmail.com>
---
crypto/ecc.c | 22 +++++++++++-----------
crypto/ecc.h | 6 ++++--
crypto/ecdh.c | 4 ++--
3 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/crypto/ecc.c b/crypto/ecc.c
index 633a9bc..9501a56 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -52,14 +52,14 @@ static inline const struct ecc_curve *ecc_get_curve(unsigned int curve_id)
}
}
-static u64 *ecc_alloc_digits_space(unsigned int ndigits)
+static u64 *ecc_alloc_digits_space(unsigned int ndigits, gfp_t gfp)
{
size_t len = ndigits * sizeof(u64);
if (!len)
return NULL;
- return kmalloc(len, GFP_KERNEL);
+ return kmalloc(len, gfp);
}
static void ecc_free_digits_space(u64 *space)
@@ -67,18 +67,18 @@ static void ecc_free_digits_space(u64 *space)
kzfree(space);
}
-static struct ecc_point *ecc_alloc_point(unsigned int ndigits)
+static struct ecc_point *ecc_alloc_point(unsigned int ndigits, gfp_t gfp)
{
- struct ecc_point *p = kmalloc(sizeof(*p), GFP_KERNEL);
+ struct ecc_point *p = kmalloc(sizeof(*p), gfp);
if (!p)
return NULL;
- p->x = ecc_alloc_digits_space(ndigits);
+ p->x = ecc_alloc_digits_space(ndigits, gfp);
if (!p->x)
goto err_alloc_x;
- p->y = ecc_alloc_digits_space(ndigits);
+ p->y = ecc_alloc_digits_space(ndigits, gfp);
if (!p->y)
goto err_alloc_y;
@@ -984,7 +984,7 @@ int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey)
}
int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
- const u64 *private_key, u64 *public_key)
+ const u64 *private_key, u64 *public_key, gfp_t gfp)
{
int ret = 0;
struct ecc_point *pk;
@@ -998,7 +998,7 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
ecc_swap_digits(private_key, priv, ndigits);
- pk = ecc_alloc_point(ndigits);
+ pk = ecc_alloc_point(ndigits, gfp);
if (!pk) {
ret = -ENOMEM;
goto out;
@@ -1021,7 +1021,7 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned int ndigits,
int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
const u64 *private_key, const u64 *public_key,
- u64 *secret)
+ u64 *secret, gfp_t gfp)
{
int ret = 0;
struct ecc_point *product, *pk;
@@ -1039,13 +1039,13 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
get_random_bytes(rand_z, nbytes);
- pk = ecc_alloc_point(ndigits);
+ pk = ecc_alloc_point(ndigits, gfp);
if (!pk) {
ret = -ENOMEM;
goto out;
}
- product = ecc_alloc_point(ndigits);
+ product = ecc_alloc_point(ndigits, gfp);
if (!product) {
ret = -ENOMEM;
goto err_alloc_product;
diff --git a/crypto/ecc.h b/crypto/ecc.h
index b35855b..f189ceb 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -62,12 +62,13 @@ int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey);
* @ndigits: curve's number of digits
* @private_key: pregenerated private key for the given curve
* @public_key: buffer for storing the generated public key
+ * @gfp: GFP flags
*
* Returns 0 if the public key was generated successfully, a negative value
* if an error occurred.
*/
int ecc_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
- const u64 *private_key, u64 *public_key);
+ const u64 *private_key, u64 *public_key, gfp_t gfp);
/**
* crypto_ecdh_shared_secret() - Compute a shared secret
@@ -77,6 +78,7 @@ int ecc_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
* @private_key: private key of part A
* @public_key: public key of counterpart B
* @secret: buffer for storing the calculated shared secret
+ * @gfp: GFP flags
*
* Note: It is recommended that you hash the result of crypto_ecdh_shared_secret
* before using it for symmetric encryption or HMAC.
@@ -86,5 +88,5 @@ int ecc_make_pub_key(const unsigned int curve_id, unsigned int ndigits,
*/
int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
const u64 *private_key, const u64 *public_key,
- u64 *secret);
+ u64 *secret, gfp_t gfp);
#endif
diff --git a/crypto/ecdh.c b/crypto/ecdh.c
index e258ab3..6a8e2f5 100644
--- a/crypto/ecdh.c
+++ b/crypto/ecdh.c
@@ -119,12 +119,12 @@ static int ecdh_compute_value(struct kpp_request *req)
ret = crypto_ecdh_shared_secret(ctx->curve_id, ctx->ndigits,
ctx->private_key, public_key,
- shared_secret);
+ shared_secret, gfp);
buf = shared_secret;
} else {
ret = ecc_make_pub_key(ctx->curve_id, ctx->ndigits,
- ctx->private_key, public_key);
+ ctx->private_key, public_key, gfp);
buf = public_key;
nbytes = public_key_sz;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread