| Message ID | 20240301022007.344948-2-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add support for NIST P521 to ecdsa | expand |
On Fri Mar 1, 2024 at 4:19 AM EET, Stefan Berger wrote: > For NIST P192/256/384 the public key's x and y parameters could be copied > directly from a given array since both parameters filled 'ndigits' of > digits (a 'digit' is a u64). For support of NIST P521 the key parameters > need to have leading zeros prepended to the most significant digit since > only 2 bytes of the most significant digit are provided. > > Therefore, implement ecc_digits_from_bytes to convert a byte array into an > array of digits and use this function in ecdsa_set_pub_key where an input > byte array needs to be converted into digits. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > crypto/ecdsa.c | 14 +++++++++----- > include/crypto/internal/ecc.h | 25 +++++++++++++++++++++++++ > 2 files changed, 34 insertions(+), 5 deletions(-) > > diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c > index fbd76498aba8..6653dec17327 100644 > --- a/crypto/ecdsa.c > +++ b/crypto/ecdsa.c > @@ -222,9 +222,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx) > static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen) > { > struct ecc_ctx *ctx = akcipher_tfm_ctx(tfm); > + unsigned int digitlen, ndigits; > const unsigned char *d = key; > - const u64 *digits = (const u64 *)&d[1]; > - unsigned int ndigits; > int ret; > > ret = ecdsa_ecc_ctx_reset(ctx); > @@ -238,12 +237,17 @@ static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsig > return -EINVAL; > > keylen--; > - ndigits = (keylen >> 1) / sizeof(u64); > + digitlen = keylen >> 1; > + > + ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); > if (ndigits != ctx->curve->g.ndigits) > return -EINVAL; > > - ecc_swap_digits(digits, ctx->pub_key.x, ndigits); > - ecc_swap_digits(&digits[ndigits], ctx->pub_key.y, ndigits); > + d++; > + > + ecc_digits_from_bytes(d, digitlen, ctx->pub_key.x, ndigits); > + ecc_digits_from_bytes(&d[digitlen], digitlen, ctx->pub_key.y, ndigits); > + > ret = ecc_is_pubkey_valid_full(ctx->curve, &ctx->pub_key); > > ctx->pub_key_set = ret == 0; > diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h > index 4f6c1a68882f..48a04605da7f 100644 > --- a/include/crypto/internal/ecc.h > +++ b/include/crypto/internal/ecc.h > @@ -56,6 +56,31 @@ static inline void ecc_swap_digits(const void *in, u64 *out, unsigned int ndigit > out[i] = get_unaligned_be64(&src[ndigits - 1 - i]); > } > > +/** > + * ecc_digits_from_bytes() - Create ndigits-sized digits array from byte array > + * @in: Input byte array > + * @nbytes Size of input byte array > + * @out Output digits array > + * @ndigits: Number of digits to create from byte array > + */ > +static inline void ecc_digits_from_bytes(const u8 *in, unsigned int nbytes, > + u64 *out, unsigned int ndigits) > +{ > + unsigned int o = nbytes & 7; > + u64 msd = 0; > + size_t i; > + > + if (o == 0) { > + ecc_swap_digits(in, out, ndigits); > + } else { > + /* if key length is not a multiple of 64 bits (NIST P521) */ > + for (i = 0; i < o; i++) > + msd = (msd << 8) | in[i]; > + out[ndigits - 1] = msd; > + ecc_swap_digits(&in[o], out, (nbytes - o) >> 3); > + } https://lore.kernel.org/keyrings/CZIOY02QS2QC.LV0A0HNT7VKM@suppilovahvero/ BR, Jarkko
On Thu, Feb 29, 2024 at 09:19:56PM -0500, Stefan Berger wrote: > --- a/crypto/ecdsa.c > +++ b/crypto/ecdsa.c > @@ -222,9 +222,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx) > static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen) > { > struct ecc_ctx *ctx = akcipher_tfm_ctx(tfm); > + unsigned int digitlen, ndigits; > const unsigned char *d = key; > - const u64 *digits = (const u64 *)&d[1]; > - unsigned int ndigits; > int ret; > > ret = ecdsa_ecc_ctx_reset(ctx); Hm, the removal of digits isn't strictly necessary. If you would keep it, the patch would become simpler (fewer lines changes). > @@ -238,12 +237,17 @@ static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsig > return -EINVAL; > > keylen--; > - ndigits = (keylen >> 1) / sizeof(u64); > + digitlen = keylen >> 1; > + > + ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); Instead of introducing an additional digitlen variable, you could just use keylen. It seems it's not used in the remainder of the function, so modifying it is harmless: keylen--; + keylen >>= 1; - ndigits = (keylen >> 1) / sizeof(u64); + ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); Just a suggestion. Thanks, Lukas
On Sat, 2024-03-02 at 22:34 +0100, Lukas Wunner wrote: > On Thu, Feb 29, 2024 at 09:19:56PM -0500, Stefan Berger wrote: [...] > > @@ -238,12 +237,17 @@ static int ecdsa_set_pub_key(struct > > crypto_akcipher *tfm, const void *key, unsig > > return -EINVAL; > > > > keylen--; > > - ndigits = (keylen >> 1) / sizeof(u64); > > + digitlen = keylen >> 1; > > + > > + ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); > > Instead of introducing an additional digitlen variable, you could > just use keylen. It seems it's not used in the remainder of the > function, so modifying it is harmless: > > keylen--; > + keylen >>= 1; > - ndigits = (keylen >> 1) / sizeof(u64); > + ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); > > Just a suggestion. The compiler will optimize the variables like this anyway (reuse registers or frames after a current consumer becomes unused) so there's no requirement to do this for efficiency, the only real question is whether using digitlen is clearer than reusing keylen, which I'll leave to the author. James
On 3/2/24 16:34, Lukas Wunner wrote: > On Thu, Feb 29, 2024 at 09:19:56PM -0500, Stefan Berger wrote: >> --- a/crypto/ecdsa.c >> +++ b/crypto/ecdsa.c >> @@ -222,9 +222,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx) >> static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen) >> { >> struct ecc_ctx *ctx = akcipher_tfm_ctx(tfm); >> + unsigned int digitlen, ndigits; >> const unsigned char *d = key; >> - const u64 *digits = (const u64 *)&d[1]; >> - unsigned int ndigits; >> int ret; >> >> ret = ecdsa_ecc_ctx_reset(ctx); > > Hm, the removal of digits isn't strictly necessary. If you would keep it, > the patch would become simpler (fewer lines changes). > > >> @@ -238,12 +237,17 @@ static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsig >> return -EINVAL; >> >> keylen--; >> - ndigits = (keylen >> 1) / sizeof(u64); >> + digitlen = keylen >> 1; >> + >> + ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); > > Instead of introducing an additional digitlen variable, you could just > use keylen. It seems it's not used in the remainder of the function, > so modifying it is harmless: > > keylen--; > + keylen >>= 1; > - ndigits = (keylen >> 1) / sizeof(u64); > + ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); > > Just a suggestion. I would prefer 'digitlen' rather than repurposing keylen and giving it a different meaning... Stefan > > Thanks, > > Lukas
diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c index fbd76498aba8..6653dec17327 100644 --- a/crypto/ecdsa.c +++ b/crypto/ecdsa.c @@ -222,9 +222,8 @@ static int ecdsa_ecc_ctx_reset(struct ecc_ctx *ctx) static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsigned int keylen) { struct ecc_ctx *ctx = akcipher_tfm_ctx(tfm); + unsigned int digitlen, ndigits; const unsigned char *d = key; - const u64 *digits = (const u64 *)&d[1]; - unsigned int ndigits; int ret; ret = ecdsa_ecc_ctx_reset(ctx); @@ -238,12 +237,17 @@ static int ecdsa_set_pub_key(struct crypto_akcipher *tfm, const void *key, unsig return -EINVAL; keylen--; - ndigits = (keylen >> 1) / sizeof(u64); + digitlen = keylen >> 1; + + ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); if (ndigits != ctx->curve->g.ndigits) return -EINVAL; - ecc_swap_digits(digits, ctx->pub_key.x, ndigits); - ecc_swap_digits(&digits[ndigits], ctx->pub_key.y, ndigits); + d++; + + ecc_digits_from_bytes(d, digitlen, ctx->pub_key.x, ndigits); + ecc_digits_from_bytes(&d[digitlen], digitlen, ctx->pub_key.y, ndigits); + ret = ecc_is_pubkey_valid_full(ctx->curve, &ctx->pub_key); ctx->pub_key_set = ret == 0; diff --git a/include/crypto/internal/ecc.h b/include/crypto/internal/ecc.h index 4f6c1a68882f..48a04605da7f 100644 --- a/include/crypto/internal/ecc.h +++ b/include/crypto/internal/ecc.h @@ -56,6 +56,31 @@ static inline void ecc_swap_digits(const void *in, u64 *out, unsigned int ndigit out[i] = get_unaligned_be64(&src[ndigits - 1 - i]); } +/** + * ecc_digits_from_bytes() - Create ndigits-sized digits array from byte array + * @in: Input byte array + * @nbytes Size of input byte array + * @out Output digits array + * @ndigits: Number of digits to create from byte array + */ +static inline void ecc_digits_from_bytes(const u8 *in, unsigned int nbytes, + u64 *out, unsigned int ndigits) +{ + unsigned int o = nbytes & 7; + u64 msd = 0; + size_t i; + + if (o == 0) { + ecc_swap_digits(in, out, ndigits); + } else { + /* if key length is not a multiple of 64 bits (NIST P521) */ + for (i = 0; i < o; i++) + msd = (msd << 8) | in[i]; + out[ndigits - 1] = msd; + ecc_swap_digits(&in[o], out, (nbytes - o) >> 3); + } +} + /** * ecc_is_key_valid() - Validate a given ECDH private key *
For NIST P192/256/384 the public key's x and y parameters could be copied directly from a given array since both parameters filled 'ndigits' of digits (a 'digit' is a u64). For support of NIST P521 the key parameters need to have leading zeros prepended to the most significant digit since only 2 bytes of the most significant digit are provided. Therefore, implement ecc_digits_from_bytes to convert a byte array into an array of digits and use this function in ecdsa_set_pub_key where an input byte array needs to be converted into digits. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- crypto/ecdsa.c | 14 +++++++++----- include/crypto/internal/ecc.h | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-)