Re: [v3 RFC PATCH 1/2] crypto: ecdh: fix concurrency on ecdh_ctx
On Tue, Jul 18, 2017 at 03:52:19PM +0300, Tudor Ambarus wrote: > > I'm replacing a per-tfm shared secret with a per-request dynamically > allocated shared secret. The same for the public key, I'm replacing > a per-tfm public key with a per-request dynamically allocated public > key. No shared memory, no concurrency. For the pre-request bits that's fine. But in setkey you're still allocating a per-tfm data structure instead of just putting things into the tfm context. This is totally pointless. > For the private key I wanted to be sure that we don't interleave data > from multiple setkey calls. As of now, the setkey calls can fight to > memcopy to the same zone of memory. I'm making sure that the private key > is valid and not a mash of multiple private keys. As I said, if the caller is making simultaneous setkey calls then it's broken. You can never fix that in your driver, so don't even try. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v3 RFC PATCH 1/2] crypto: ecdh: fix concurrency on ecdh_ctx
Hi, Herbert, On 07/18/2017 08:50 AM, Herbert Xu wrote: On Wed, Jun 28, 2017 at 05:08:35PM +0300, 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. I don't get it. You're replacing a per-tfm shared secret with a per-tfm dynamically allocated shared secret. As far as I can see nothing has changed? I'm replacing a per-tfm shared secret with a per-request dynamically allocated shared secret. The same for the public key, I'm replacing a per-tfm public key with a per-request dynamically allocated public key. No shared memory, no concurrency. If the caller is making simultaneous setkey calls on the same tfm, then it's their problem. For the private key I wanted to be sure that we don't interleave data from multiple setkey calls. As of now, the setkey calls can fight to memcopy to the same zone of memory. I'm making sure that the private key is valid and not a mash of multiple private keys. Thanks, ta
Re: [v3 RFC PATCH 1/2] crypto: ecdh: fix concurrency on ecdh_ctx
On Wed, Jun 28, 2017 at 05:08:35PM +0300, 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. I don't get it. You're replacing a per-tfm shared secret with a per-tfm dynamically allocated shared secret. As far as I can see nothing has changed? If the caller is making simultaneous setkey calls on the same tfm, then it's their problem. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[v3 RFC PATCH 1/2] crypto: ecdh: fix concurrency on ecdh_ctx
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--- 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, ) < 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) { -