| Message ID | 20240306222257.979304-10-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for NIST P521 to ecdsa | expand |
On Thu Mar 7, 2024 at 12:22 AM EET, Stefan Berger wrote: > In some cases the name keylen does not reflect the purpose of the variable > anymore once NIST P521 is used but it is the size of the buffer. There- > for, rename keylen to bufsize where appropriate. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > Tested-by: Lukas Wunner <lukas@wunner.de> > --- > crypto/ecdsa.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c > index 4daefb40c37a..4e847b59622a 100644 > --- a/crypto/ecdsa.c > +++ b/crypto/ecdsa.c > @@ -35,8 +35,8 @@ struct ecdsa_signature_ctx { > static int ecdsa_get_signature_rs(u64 *dest, size_t hdrlen, unsigned char tag, > const void *value, size_t vlen, unsigned int ndigits) > { > - size_t keylen = ndigits * sizeof(u64); nit: still don't get why "* sizeof(u64)" would ever be more readable thean "* 8". BR, Jarkko
On 3/7/24 14:13, Jarkko Sakkinen wrote: > On Thu Mar 7, 2024 at 12:22 AM EET, Stefan Berger wrote: >> In some cases the name keylen does not reflect the purpose of the variable >> anymore once NIST P521 is used but it is the size of the buffer. There- >> for, rename keylen to bufsize where appropriate. >> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> Tested-by: Lukas Wunner <lukas@wunner.de> >> --- >> crypto/ecdsa.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c >> index 4daefb40c37a..4e847b59622a 100644 >> --- a/crypto/ecdsa.c >> +++ b/crypto/ecdsa.c >> @@ -35,8 +35,8 @@ struct ecdsa_signature_ctx { >> static int ecdsa_get_signature_rs(u64 *dest, size_t hdrlen, unsigned char tag, >> const void *value, size_t vlen, unsigned int ndigits) >> { >> - size_t keylen = ndigits * sizeof(u64); > > nit: still don't get why "* sizeof(u64)" would ever be more readable > thean "* 8". Because existing code in crypto uses sizeof(u64) when converting ndigits to number of bytes and '8' is not used for converting to bytes. Do we need to change this now ? No, I think it's better to conform to existing code. # grep -rI ndigits crypto/ | grep sizeof\(u64\) crypto/ecrdsa.c: unsigned int ndigits = req->dst_len / sizeof(u64); crypto/ecrdsa.c: req->dst_len != ctx->curve->g.ndigits * sizeof(u64) || crypto/ecrdsa.c: vli_from_be64(r, sig + ndigits * sizeof(u64), ndigits); crypto/ecrdsa.c: ctx->curve->g.ndigits * sizeof(u64) != ctx->digest_len) crypto/ecrdsa.c: ctx->key_len != ctx->curve->g.ndigits * sizeof(u64) * 2) crypto/ecrdsa.c: ndigits = ctx->key_len / sizeof(u64) / 2; crypto/ecrdsa.c: vli_from_le64(ctx->pub_key.y, ctx->key + ndigits * sizeof(u64), crypto/ecrdsa.c: return ctx->pub_key.ndigits * sizeof(u64); crypto/ecdh.c: params.key_size > sizeof(u64) * ctx->ndigits) crypto/ecc.c: size_t len = ndigits * sizeof(u64); crypto/ecc.c: num_bits = sizeof(u64) * ndigits * 8 + 1; crypto/ecdsa.c: size_t bufsize = ndigits * sizeof(u64); crypto/ecdsa.c: size_t bufsize = ctx->curve->g.ndigits * sizeof(u64); crypto/ecdsa.c: ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); Stefan > > BR, Jarkko
Jarkko, On Thu, Mar 07, 2024 at 02:20:12PM -0500, Stefan Berger wrote: > On 3/7/24 14:13, Jarkko Sakkinen wrote: > > On Thu Mar 7, 2024 at 12:22 AM EET, Stefan Berger wrote: > > > In some cases the name keylen does not reflect the purpose of the variable > > > anymore once NIST P521 is used but it is the size of the buffer. There- > > > for, rename keylen to bufsize where appropriate. > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > > Tested-by: Lukas Wunner <lukas@wunner.de> > > > --- > > > crypto/ecdsa.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c > > > index 4daefb40c37a..4e847b59622a 100644 > > > --- a/crypto/ecdsa.c > > > +++ b/crypto/ecdsa.c > > > @@ -35,8 +35,8 @@ struct ecdsa_signature_ctx { > > > static int ecdsa_get_signature_rs(u64 *dest, size_t hdrlen, unsigned char tag, > > > const void *value, size_t vlen, unsigned int ndigits) > > > { > > > - size_t keylen = ndigits * sizeof(u64); > > > > nit: still don't get why "* sizeof(u64)" would ever be more readable > > thean "* 8". > > Because existing code in crypto uses sizeof(u64) when converting ndigits to > number of bytes and '8' is not used for converting to bytes. Do we need to > change this now ? No, I think it's better to conform to existing code. `sizeof(u64)` is easily read as `8` by reviewers, but just `8` will require inline comments because it's magic number isn't it? So this will not even decrease number of letters. `sizeof(u64)` is self-documenting code and you don't even need to interpret it as `8` for review as you don't need number from any sizeof(struct ..). Also, possible we need to calculate number of bits in the big number, so this would become `* 8 * 8`, in that case how would you distinguish omission of one `* 8` by a typo. Overall it's quite common in the whole tree linux/torvalds$ git grep -e '\* sizeof(u64)' -e 'sizeof(u64) \*' | wc -l 551 So this is perhaps acceptable and depends on point of view. crypto subsystem coders seems to prefer not to save on letters and type `sizeof(u64)`. Thanks, > > # grep -rI ndigits crypto/ | grep sizeof\(u64\) > crypto/ecrdsa.c: unsigned int ndigits = req->dst_len / sizeof(u64); > crypto/ecrdsa.c: req->dst_len != ctx->curve->g.ndigits * > sizeof(u64) || > crypto/ecrdsa.c: vli_from_be64(r, sig + ndigits * sizeof(u64), > ndigits); > crypto/ecrdsa.c: ctx->curve->g.ndigits * sizeof(u64) != > ctx->digest_len) > crypto/ecrdsa.c: ctx->key_len != ctx->curve->g.ndigits * > sizeof(u64) * 2) > crypto/ecrdsa.c: ndigits = ctx->key_len / sizeof(u64) / 2; > crypto/ecrdsa.c: vli_from_le64(ctx->pub_key.y, ctx->key + ndigits * > sizeof(u64), > crypto/ecrdsa.c: return ctx->pub_key.ndigits * sizeof(u64); > crypto/ecdh.c: params.key_size > sizeof(u64) * ctx->ndigits) > crypto/ecc.c: size_t len = ndigits * sizeof(u64); > crypto/ecc.c: num_bits = sizeof(u64) * ndigits * 8 + 1; > crypto/ecdsa.c: size_t bufsize = ndigits * sizeof(u64); > crypto/ecdsa.c: size_t bufsize = ctx->curve->g.ndigits * sizeof(u64); > crypto/ecdsa.c: ndigits = DIV_ROUND_UP(digitlen, sizeof(u64)); > > Stefan > > > > > BR, Jarkko
diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c index 4daefb40c37a..4e847b59622a 100644 --- a/crypto/ecdsa.c +++ b/crypto/ecdsa.c @@ -35,8 +35,8 @@ struct ecdsa_signature_ctx { static int ecdsa_get_signature_rs(u64 *dest, size_t hdrlen, unsigned char tag, const void *value, size_t vlen, unsigned int ndigits) { - size_t keylen = ndigits * sizeof(u64); - ssize_t diff = vlen - keylen; + size_t bufsize = ndigits * sizeof(u64); + ssize_t diff = vlen - bufsize; const char *d = value; u8 rs[ECC_MAX_BYTES]; @@ -58,7 +58,7 @@ static int ecdsa_get_signature_rs(u64 *dest, size_t hdrlen, unsigned char tag, if (diff) return -EINVAL; } - if (-diff >= keylen) + if (-diff >= bufsize) return -EINVAL; if (diff) { @@ -138,7 +138,7 @@ static int ecdsa_verify(struct akcipher_request *req) { struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req); struct ecc_ctx *ctx = akcipher_tfm_ctx(tfm); - size_t keylen = ctx->curve->g.ndigits * sizeof(u64); + size_t bufsize = ctx->curve->g.ndigits * sizeof(u64); struct ecdsa_signature_ctx sig_ctx = { .curve = ctx->curve, }; @@ -165,14 +165,14 @@ static int ecdsa_verify(struct akcipher_request *req) goto error; /* if the hash is shorter then we will add leading zeros to fit to ndigits */ - diff = keylen - req->dst_len; + diff = bufsize - req->dst_len; if (diff >= 0) { if (diff) memset(rawhash, 0, diff); memcpy(&rawhash[diff], buffer + req->src_len, req->dst_len); } else if (diff < 0) { /* given hash is longer, we take the left-most bytes */ - memcpy(&rawhash, buffer + req->src_len, keylen); + memcpy(&rawhash, buffer + req->src_len, bufsize); } ecc_swap_digits((u64 *)rawhash, hash, ctx->curve->g.ndigits);