| Message ID | 20240529230827.379111-2-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ecdsa: Use ecc_digits_from_bytes to simplify code | expand |
On Thu May 30, 2024 at 2:08 AM EEST, Stefan Berger wrote: > Since ecc_digits_from_bytes will provide zeros when an insufficient number > of bytes are passed in the input byte array, use it to create the hash > digits directly from the input byte array. This avoids going through an > intermediate byte array (rawhash) that has the first few bytes filled with > zeros. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > crypto/ecdsa.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c > index 258fffbf623d..fa029f36110b 100644 > --- a/crypto/ecdsa.c > +++ b/crypto/ecdsa.c > @@ -142,10 +142,8 @@ static int ecdsa_verify(struct akcipher_request *req) > struct ecdsa_signature_ctx sig_ctx = { > .curve = ctx->curve, > }; > - u8 rawhash[ECC_MAX_BYTES]; > u64 hash[ECC_MAX_DIGITS]; > unsigned char *buffer; > - ssize_t diff; > int ret; > > if (unlikely(!ctx->pub_key_set)) > @@ -164,18 +162,11 @@ static int ecdsa_verify(struct akcipher_request *req) > if (ret < 0) > goto error; > > - /* if the hash is shorter then we will add leading zeros to fit to ndigits */ > - 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, bufsize); > - } > + if (bufsize > req->dst_len) > + bufsize = req->dst_len; > > - ecc_swap_digits((u64 *)rawhash, hash, ctx->curve->g.ndigits); > + ecc_digits_from_bytes(buffer + req->src_len, bufsize, > + hash, ctx->curve->g.ndigits); > > ret = _ecdsa_verify(ctx, hash, sig_ctx.r, sig_ctx.s); > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> I don't think it'd be even nit-picking to say that the function called would really need kdoc. I had to spend about 20 minutes to reacall ecc_digits_from_bytes(). Like something to remind what, how and why... So that you can recap quickly. Once I got grip of it (for the 2nd time) the code itself was just fine, no complains on that. BR, Jarkko
On 5/30/24 01:28, Jarkko Sakkinen wrote: > On Thu May 30, 2024 at 2:08 AM EEST, Stefan Berger wrote: >> Since ecc_digits_from_bytes will provide zeros when an insufficient number >> of bytes are passed in the input byte array, use it to create the hash >> digits directly from the input byte array. This avoids going through an >> intermediate byte array (rawhash) that has the first few bytes filled with >> zeros. >> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> --- >> crypto/ecdsa.c | 17 ++++------------- >> 1 file changed, 4 insertions(+), 13 deletions(-) >> >> diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c >> index 258fffbf623d..fa029f36110b 100644 >> --- a/crypto/ecdsa.c >> +++ b/crypto/ecdsa.c >> @@ -142,10 +142,8 @@ static int ecdsa_verify(struct akcipher_request *req) >> struct ecdsa_signature_ctx sig_ctx = { >> .curve = ctx->curve, >> }; >> - u8 rawhash[ECC_MAX_BYTES]; >> u64 hash[ECC_MAX_DIGITS]; >> unsigned char *buffer; >> - ssize_t diff; >> int ret; >> >> if (unlikely(!ctx->pub_key_set)) >> @@ -164,18 +162,11 @@ static int ecdsa_verify(struct akcipher_request *req) >> if (ret < 0) >> goto error; >> >> - /* if the hash is shorter then we will add leading zeros to fit to ndigits */ >> - 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, bufsize); >> - } >> + if (bufsize > req->dst_len) >> + bufsize = req->dst_len; >> >> - ecc_swap_digits((u64 *)rawhash, hash, ctx->curve->g.ndigits); >> + ecc_digits_from_bytes(buffer + req->src_len, bufsize, >> + hash, ctx->curve->g.ndigits); >> >> ret = _ecdsa_verify(ctx, hash, sig_ctx.r, sig_ctx.s); >> > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> > > I don't think it'd be even nit-picking to say that the function > called would really need kdoc. I had to spend about 20 minutes > to reacall ecc_digits_from_bytes(). Here's the file with all the kdocs: https://elixir.bootlin.com/linux/v6.10-rc1/source/include/crypto/internal/ecc.h#L67 > > Like something to remind what, how and why... So that you can > recap quickly. Once I got grip of it (for the 2nd time) the > code itself was just fine, no complains on that. Do you want to find there that the input byte array starts with the most significant byte and the functions converts this byte array into an internal digits representation? > > BR, Jarkko >
On Thu May 30, 2024 at 3:24 PM EEST, Stefan Berger wrote: > > > On 5/30/24 01:28, Jarkko Sakkinen wrote: > > On Thu May 30, 2024 at 2:08 AM EEST, Stefan Berger wrote: > >> Since ecc_digits_from_bytes will provide zeros when an insufficient number > >> of bytes are passed in the input byte array, use it to create the hash > >> digits directly from the input byte array. This avoids going through an > >> intermediate byte array (rawhash) that has the first few bytes filled with > >> zeros. > >> > >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > >> --- > >> crypto/ecdsa.c | 17 ++++------------- > >> 1 file changed, 4 insertions(+), 13 deletions(-) > >> > >> diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c > >> index 258fffbf623d..fa029f36110b 100644 > >> --- a/crypto/ecdsa.c > >> +++ b/crypto/ecdsa.c > >> @@ -142,10 +142,8 @@ static int ecdsa_verify(struct akcipher_request *req) > >> struct ecdsa_signature_ctx sig_ctx = { > >> .curve = ctx->curve, > >> }; > >> - u8 rawhash[ECC_MAX_BYTES]; > >> u64 hash[ECC_MAX_DIGITS]; > >> unsigned char *buffer; > >> - ssize_t diff; > >> int ret; > >> > >> if (unlikely(!ctx->pub_key_set)) > >> @@ -164,18 +162,11 @@ static int ecdsa_verify(struct akcipher_request *req) > >> if (ret < 0) > >> goto error; > >> > >> - /* if the hash is shorter then we will add leading zeros to fit to ndigits */ > >> - 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, bufsize); > >> - } > >> + if (bufsize > req->dst_len) > >> + bufsize = req->dst_len; > >> > >> - ecc_swap_digits((u64 *)rawhash, hash, ctx->curve->g.ndigits); > >> + ecc_digits_from_bytes(buffer + req->src_len, bufsize, > >> + hash, ctx->curve->g.ndigits); > >> > >> ret = _ecdsa_verify(ctx, hash, sig_ctx.r, sig_ctx.s); > >> > > > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > I don't think it'd be even nit-picking to say that the function > > called would really need kdoc. I had to spend about 20 minutes > > to reacall ecc_digits_from_bytes(). > > Here's the file with all the kdocs: > https://elixir.bootlin.com/linux/v6.10-rc1/source/include/crypto/internal/ecc.h#L67 LOL, sorry I forgot this. I think this was the 2nd time I complained about this ;-) I'm sorry, yeah that WFM. Just not used to this convention but I don't mind actually if it is that way. BR, Jarkko
diff --git a/crypto/ecdsa.c b/crypto/ecdsa.c index 258fffbf623d..fa029f36110b 100644 --- a/crypto/ecdsa.c +++ b/crypto/ecdsa.c @@ -142,10 +142,8 @@ static int ecdsa_verify(struct akcipher_request *req) struct ecdsa_signature_ctx sig_ctx = { .curve = ctx->curve, }; - u8 rawhash[ECC_MAX_BYTES]; u64 hash[ECC_MAX_DIGITS]; unsigned char *buffer; - ssize_t diff; int ret; if (unlikely(!ctx->pub_key_set)) @@ -164,18 +162,11 @@ static int ecdsa_verify(struct akcipher_request *req) if (ret < 0) goto error; - /* if the hash is shorter then we will add leading zeros to fit to ndigits */ - 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, bufsize); - } + if (bufsize > req->dst_len) + bufsize = req->dst_len; - ecc_swap_digits((u64 *)rawhash, hash, ctx->curve->g.ndigits); + ecc_digits_from_bytes(buffer + req->src_len, bufsize, + hash, ctx->curve->g.ndigits); ret = _ecdsa_verify(ctx, hash, sig_ctx.r, sig_ctx.s);
Since ecc_digits_from_bytes will provide zeros when an insufficient number of bytes are passed in the input byte array, use it to create the hash digits directly from the input byte array. This avoids going through an intermediate byte array (rawhash) that has the first few bytes filled with zeros. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- crypto/ecdsa.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)