| Message ID | 1495034813-27143-2-git-send-email-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
Am Mittwoch, 17. Mai 2017, 17:26:50 CEST schrieb Tudor Ambarus: Hi Tudor, > Add support for generating ecc private keys. > > Generation of ecc private keys is helpful in a user-space to kernel > ecdh offload because the keys are not revealed to user-space. Private > key generation is also helpful to implement forward secrecy. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > crypto/ecc.c | 20 ++++++++++++++++++++ > crypto/ecc.h | 14 ++++++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/crypto/ecc.c b/crypto/ecc.c > index 414c78a..a591907 100644 > --- a/crypto/ecc.c > +++ b/crypto/ecc.c > @@ -927,6 +927,26 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned > int ndigits, return 0; > } > > +int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 > *privkey) +{ > + const struct ecc_curve *curve = ecc_get_curve(curve_id); Shouldn't there be a check that a curve is selected? I.e. a check for an error should be added? > + u64 priv[ndigits]; Shouldn't there be a size check of ndigits? > + unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT; > + > + get_random_bytes(priv, nbytes); Can you please use crypto_get_default_rng / crypto_rng_get_bytes / crypto_put_default_rng? > + > + if (vli_is_zero(priv, ndigits)) > + return -EINVAL; > + > + /* Make sure the private key is in the range [1, n-1]. */ > + if (vli_cmp(curve->n, priv, ndigits) != 1) > + return -EINVAL; > + > + ecc_swap_digits(priv, privkey, ndigits); Is a byteswap faster than a copy operation by looping through priv/privkey and simply assinging the value? > + > + return 0; > +} > + > 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) > diff --git a/crypto/ecc.h b/crypto/ecc.h > index 663d598..b94b7ce 100644 > --- a/crypto/ecc.h > +++ b/crypto/ecc.h > @@ -44,6 +44,20 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int > ndigits, const u8 *private_key, unsigned int private_key_len); > > /** > + * ecc_gen_privkey() - Generates an ECC private key. > + * The private key is a random integer in the range 0 < random < n, where n > is a + * prime that is the order of the cyclic subgroup generated by the > distinguished + * point G. > + * @curve_id: id representing the curve to use > + * @ndigits: curve number of digits > + * @private_key: buffer for storing the generated private key > + * > + * Returns 0 if the private key was generated successfully, a negative > value + * if an error occurred. > + */ > +int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 > *privkey); + > +/** > * ecdh_make_pub_key() - Compute an ECC public key > * > * @curve_id: id representing the curve to use Ciao Stephan
Hi, Stephan, Thank you for the review. Please see inline. On 28.05.2017 21:44, Stephan Müller wrote: > Am Mittwoch, 17. Mai 2017, 17:26:50 CEST schrieb Tudor Ambarus: > > Hi Tudor, > >> Add support for generating ecc private keys. >> >> Generation of ecc private keys is helpful in a user-space to kernel >> ecdh offload because the keys are not revealed to user-space. Private >> key generation is also helpful to implement forward secrecy. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> crypto/ecc.c | 20 ++++++++++++++++++++ >> crypto/ecc.h | 14 ++++++++++++++ >> 2 files changed, 34 insertions(+) >> >> diff --git a/crypto/ecc.c b/crypto/ecc.c >> index 414c78a..a591907 100644 >> --- a/crypto/ecc.c >> +++ b/crypto/ecc.c >> @@ -927,6 +927,26 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned >> int ndigits, return 0; >> } >> >> +int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 >> *privkey) +{ >> + const struct ecc_curve *curve = ecc_get_curve(curve_id); > > Shouldn't there be a check that a curve is selected? I.e. a check for an error > should be added? We already checked the curve_id before generating the key. The function assumes that curve_id is checked before calling it, like ecc_is_key_valid() does. > >> + u64 priv[ndigits]; > > Shouldn't there be a size check of ndigits? Already checked when setting the secret. But FIPS 186-4, B.4.1 recommends to check the bit length of n. It should be greater or equal with 160. I will add this check, thanks. > >> + unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT; >> + >> + get_random_bytes(priv, nbytes); > > Can you please use crypto_get_default_rng / crypto_rng_get_bytes / > crypto_put_default_rng? Actually I tried this and I encountered some problems, I'm currently debugging it. When using the default rng and the run-time self tests are enabled, the kernel is in a blocking state. What's worse is that the kernel blocks before the console has the chance to be enabled and I can't see anything :). I suspect that the kernel blocks because the rng does not have enough entropy. Could you please give me some hints? >> + >> + if (vli_is_zero(priv, ndigits)) >> + return -EINVAL; >> + >> + /* Make sure the private key is in the range [1, n-1]. */ >> + if (vli_cmp(curve->n, priv, ndigits) != 1) >> + return -EINVAL; >> + >> + ecc_swap_digits(priv, privkey, ndigits); > > Is a byteswap faster than a copy operation by looping through priv/privkey and > simply assinging the value? Maybe not, but I am consistent with the rest of the code. Can we change this in a latter patch, if necessary? Thanks, ta
Am Montag, 29. Mai 2017, 11:08:38 CEST schrieb Tudor Ambarus: Hi Tudor, > > >> + unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT; > >> + > >> + get_random_bytes(priv, nbytes); > > > > Can you please use crypto_get_default_rng / crypto_rng_get_bytes / > > crypto_put_default_rng? > > Actually I tried this and I encountered some problems, I'm currently > debugging it. > > When using the default rng and the run-time self tests are enabled, > the kernel is in a blocking state. What's worse is that the kernel > blocks before the console has the chance to be enabled and I can't see > anything :). > > I suspect that the kernel blocks because the rng does not have enough > entropy. Could you please give me some hints? Hm, there should be no blocking for the DRBG to initialize. What happens if you compile that as a module and insmod it at runtime? > > >> + > >> + if (vli_is_zero(priv, ndigits)) > >> + return -EINVAL; > >> + > >> + /* Make sure the private key is in the range [1, n-1]. */ > >> + if (vli_cmp(curve->n, priv, ndigits) != 1) > >> + return -EINVAL; > >> + > >> + ecc_swap_digits(priv, privkey, ndigits); > > > > Is a byteswap faster than a copy operation by looping through priv/privkey > > and simply assinging the value? > > Maybe not, but I am consistent with the rest of the code. Can we change > this in a latter patch, if necessary? Ok, fine with me. Ciao Stephan
Hi, Stephan, On 29.05.2017 12:23, Stephan Müller wrote: > Am Montag, 29. Mai 2017, 11:08:38 CEST schrieb Tudor Ambarus: > > Hi Tudor, >> >>>> + unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT; >>>> + >>>> + get_random_bytes(priv, nbytes); >>> >>> Can you please use crypto_get_default_rng / crypto_rng_get_bytes / >>> crypto_put_default_rng? >> >> Actually I tried this and I encountered some problems, I'm currently >> debugging it. >> >> When using the default rng and the run-time self tests are enabled, >> the kernel is in a blocking state. What's worse is that the kernel >> blocks before the console has the chance to be enabled and I can't see >> anything :). >> >> I suspect that the kernel blocks because the rng does not have enough >> entropy. Could you please give me some hints? > > Hm, there should be no blocking for the DRBG to initialize. > > What happens if you compile that as a module and insmod it at runtime? We will have a nop: #ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS /* a perfect nop */ int alg_test(const char *driver, const char *alg, u32 type, u32 mask) { printk(KERN_ERR "no op in alg_test"); return 0; } #else ... #endif If I further mangle it and change #ifdef with #ifndef then the tests are passing. Thanks, ta
Am Montag, 29. Mai 2017, 11:47:48 CEST schrieb Tudor Ambarus: Hi Tudor, > > Hm, there should be no blocking for the DRBG to initialize. > > > > What happens if you compile that as a module and insmod it at runtime? > > We will have a nop: > > #ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS > > /* a perfect nop */ > int alg_test(const char *driver, const char *alg, u32 type, u32 mask) > { > printk(KERN_ERR "no op in alg_test"); > return 0; > } > > #else > ... > #endif > > If I further mangle it and change #ifdef with #ifndef then the tests are > passing. Can you please enable the testmgr in the kernel config and compile the DH/ECDH code as a module. Then I would insmod the dh/ecdh kernel module to see whether the self tests work or not. Note, if you use the calls of crypto_get_default_rng and friends, the DRBG must be present in the kernel at the time of calling. I.e. if you statically compile dh/ecdh but compile the RNG support as a module, the RNG will not load during self test. If you compile dh/ecdh as a module, the RNG can stay as a module. Ciao Stephan
Hi, Stephan, On 29.05.2017 12:56, Stephan Müller wrote: > Am Montag, 29. Mai 2017, 11:47:48 CEST schrieb Tudor Ambarus: > > Hi Tudor, > >>> Hm, there should be no blocking for the DRBG to initialize. >>> >>> What happens if you compile that as a module and insmod it at runtime? >> >> We will have a nop: >> >> #ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS >> >> /* a perfect nop */ >> int alg_test(const char *driver, const char *alg, u32 type, u32 mask) >> { >> printk(KERN_ERR "no op in alg_test"); >> return 0; >> } >> >> #else >> ... >> #endif >> >> If I further mangle it and change #ifdef with #ifndef then the tests are >> passing. > > Can you please enable the testmgr in the kernel config and compile the DH/ECDH > code as a module. Then I would insmod the dh/ecdh kernel module to see whether > the self tests work or not. dh/ecdh self tests are passing when I insert dh/ecdh modules. > > Note, if you use the calls of crypto_get_default_rng and friends, the DRBG > must be present in the kernel at the time of calling. I.e. if you statically > compile dh/ecdh but compile the RNG support as a module, the RNG will not load > during self test. If you compile dh/ecdh as a module, the RNG can stay as a > module. However, when the self tests are enabled and DH/ECDH code is built-in, the kernel blocks. I have to ensure that the drivers are loaded in the right order. Deferring the probe or just simply changing the order of the object files from crypto/Makefile solves the issue. I'll go with the second approach. Thanks, ta
diff --git a/crypto/ecc.c b/crypto/ecc.c index 414c78a..a591907 100644 --- a/crypto/ecc.c +++ b/crypto/ecc.c @@ -927,6 +927,26 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits, return 0; } +int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey) +{ + const struct ecc_curve *curve = ecc_get_curve(curve_id); + u64 priv[ndigits]; + unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT; + + get_random_bytes(priv, nbytes); + + if (vli_is_zero(priv, ndigits)) + return -EINVAL; + + /* Make sure the private key is in the range [1, n-1]. */ + if (vli_cmp(curve->n, priv, ndigits) != 1) + return -EINVAL; + + ecc_swap_digits(priv, privkey, ndigits); + + return 0; +} + 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) diff --git a/crypto/ecc.h b/crypto/ecc.h index 663d598..b94b7ce 100644 --- a/crypto/ecc.h +++ b/crypto/ecc.h @@ -44,6 +44,20 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int ndigits, const u8 *private_key, unsigned int private_key_len); /** + * ecc_gen_privkey() - Generates an ECC private key. + * The private key is a random integer in the range 0 < random < n, where n is a + * prime that is the order of the cyclic subgroup generated by the distinguished + * point G. + * @curve_id: id representing the curve to use + * @ndigits: curve number of digits + * @private_key: buffer for storing the generated private key + * + * Returns 0 if the private key was generated successfully, a negative value + * if an error occurred. + */ +int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey); + +/** * ecdh_make_pub_key() - Compute an ECC public key * * @curve_id: id representing the curve to use
Add support for generating ecc private keys. Generation of ecc private keys is helpful in a user-space to kernel ecdh offload because the keys are not revealed to user-space. Private key generation is also helpful to implement forward secrecy. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- crypto/ecc.c | 20 ++++++++++++++++++++ crypto/ecc.h | 14 ++++++++++++++ 2 files changed, 34 insertions(+)