Hi,
On 22.06.2017 14:03, Tudor Ambarus wrote:
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@xxxxxxxxxxxxx>
---
crypto/ecc.h | 2 --
crypto/ecdh.c | 93 ++++++++++++++++++++++++++++++++++++++++++-----------------
2 files changed, 67 insertions(+), 28 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..30d954d 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)
@@ -38,13 +36,31 @@ static unsigned int ecdh_supported_curve(unsigned int curve_id)
}
}
+static int ecdh_gen_privkey(struct ecdh_ctx *ctx, u64 *private_key)
+{
+ int ret;
+
+ ret = ecc_gen_privkey(ctx->curve_id, ctx->ndigits, private_key);
+ if (ret) {
+ kzfree(private_key);
It's ugly to free a pointer that I received as a function argument.
I'll get rid of this function.
+ return ret;
+ }
+
+ ctx->private_key = private_key;
+ return 0;
+}
+
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;
+ /* clear the old private key, if any */
+ kzfree(ctx->private_key);
+
if (crypto_ecdh_decode_key(buf, len, ¶ms) < 0)
return -EINVAL;
@@ -52,59 +68,82 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
if (!ndigits)
return -EINVAL;
+ private_key = kzalloc(ndigits << ECC_DIGITS_TO_BYTES_SHIFT, GFP_KERNEL);
kmalloc should suffice.
+ 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);
+ return ecdh_gen_privkey(ctx, private_key);
if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
- (const u64 *)params.key, params.key_size) < 0)
+ (const u64 *)params.key, params.key_size) < 0) {
+ kzfree(private_key);
return -EINVAL;
+ }
- memcpy(ctx->private_key, params.key, params.key_size);
+ memcpy(private_key, params.key, params.key_size);
+ ctx->private_key = private_key;
return 0;
}
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;
+ size_t copied, nbytes, public_key_sz;
void *buf;
+ 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;
+
+ public_key = kmalloc(public_key_sz, GFP_KERNEL);
It's wrong to assume that I can sleep while processing a request.
I will use the gfp_t flags from request.
+ 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_KERNEL);
same here
+ 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);
While this comment has nothing to do with this patch, I see that
crypto_ecdh_shared_secret ends up in calling ecc_alloc_digits_space,
which allocates memory with GFP_KERNEL. I will make a patch to fix
that too.
ta
