Message ID | 20240301022007.344948-1-stefanb@linux.ibm.com |
---|---|
Headers | show |
Series | Add support for NIST P521 to ecdsa | expand |
On 3/1/24 15:41, Jarkko Sakkinen wrote: > On Fri Mar 1, 2024 at 4:20 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> >> --- >> 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); >> - ssize_t diff = vlen - keylen; >> + size_t bufsize = ndigits * sizeof(u64); > > why not just "* 8"? using sizeof here makes this function only unreadable. 'unreadable' is a 'strong' word ... # grep -r -E "ndigits \*" crypto/ | grep sizeof 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: vli_from_le64(ctx->pub_key.y, ctx->key + ndigits * sizeof(u64), crypto/ecrdsa.c: return ctx->pub_key.ndigits * sizeof(u64); crypto/ecc.c: size_t len = ndigits * sizeof(u64); crypto/ecc.c: num_bits = sizeof(u64) * ndigits * 8 + 1; crypto/ecdsa.c: size_t keylen = ndigits * sizeof(u64); crypto/ecdsa.c: size_t keylen = ctx->curve->g.ndigits * sizeof(u64); Stefan
On Fri Mar 1, 2024 at 10:47 PM EET, Stefan Berger wrote: > > > On 3/1/24 15:41, Jarkko Sakkinen wrote: > > On Fri Mar 1, 2024 at 4:20 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> > >> --- > >> 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); > >> - ssize_t diff = vlen - keylen; > >> + size_t bufsize = ndigits * sizeof(u64); > > > > why not just "* 8"? using sizeof here makes this function only unreadable. > > 'unreadable' is a 'strong' word ... so what is the gain when writing sizeof(u64)? BR, Jarkko
On 3/1/24 15:50, Jarkko Sakkinen wrote: > On Fri Mar 1, 2024 at 10:47 PM EET, Stefan Berger wrote: >> >> >> On 3/1/24 15:41, Jarkko Sakkinen wrote: >>> On Fri Mar 1, 2024 at 4:20 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> >>>> --- >>>> 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); >>>> - ssize_t diff = vlen - keylen; >>>> + size_t bufsize = ndigits * sizeof(u64); >>> >>> why not just "* 8"? using sizeof here makes this function only unreadable. >> >> 'unreadable' is a 'strong' word ... > > so what is the gain when writing sizeof(u64)? It matches existing code and a digit is a 'u64'. > > BR, Jarkko
On Thu, Feb 29, 2024 at 09:19:55PM -0500, Stefan Berger wrote: > This series adds support for the NIST P521 curve to the ecdsa module > to enable signature verification with it. > > An issue with the current code in ecdsa is that it assumes that input > arrays providing key coordinates for example, are arrays of digits > (a 'digit' is a 'u64'). This works well for all currently supported > curves, such as NIST P192/256/384, but does not work for NIST P521 where > coordinates are 8 digits + 2 bytes long. So some of the changes deal with > converting byte arrays to digits and adjusting tests on input byte > array lengths to tolerate arrays not providing multiples of 8 bytes. When respinning this series as v5, feel free to add my Tested-by: Lukas Wunner <lukas@wunner.de> I cherry-picked the commits from your nist_p521.v5 branch... https://github.com/stefanberger/linux-ima-namespaces/commits/nist_p521.v5/ ...onto my development branch for PCI device authentication... https://github.com/l1k/linux/commits/doe ...and tested against qemu+libspdm that an emulated NVMe drive is able to present a valid signature using NIST P521 + SHA384 which can be verified correctly by the kernel. I needed to fix up two of my patches, one which adds P1363 signature format support to the kernel and another fixup to add NIST P521 support to the in-kernel SPDM library (two top-most commits on my above-linked development branch). I performed this test against your f81547267725 head and notice that you pushed a new version today (with "curve->nbits == 521" instead of strcmp), but I'm confident those two small changes wouldn't alter the outcone, hence my Tested-by stands. Thanks, Lukas
On 3/4/24 13:10, Lukas Wunner wrote: > On Thu, Feb 29, 2024 at 09:19:55PM -0500, Stefan Berger wrote: >> This series adds support for the NIST P521 curve to the ecdsa module >> to enable signature verification with it. >> >> An issue with the current code in ecdsa is that it assumes that input >> arrays providing key coordinates for example, are arrays of digits >> (a 'digit' is a 'u64'). This works well for all currently supported >> curves, such as NIST P192/256/384, but does not work for NIST P521 where >> coordinates are 8 digits + 2 bytes long. So some of the changes deal with >> converting byte arrays to digits and adjusting tests on input byte >> array lengths to tolerate arrays not providing multiples of 8 bytes. > > When respinning this series as v5, feel free to add my > > Tested-by: Lukas Wunner <lukas@wunner.de> Thanks. > > > I cherry-picked the commits from your nist_p521.v5 branch... > > https://github.com/stefanberger/linux-ima-namespaces/commits/nist_p521.v5/ > > ...onto my development branch for PCI device authentication... > > https://github.com/l1k/linux/commits/doe > > ...and tested against qemu+libspdm that an emulated NVMe drive > is able to present a valid signature using NIST P521 + SHA384 > which can be verified correctly by the kernel. FYI: I have a PR for a test suite here as well: https://github.com/stefanberger/eckey-testing/pull/1 > > I needed to fix up two of my patches, one which adds P1363 > signature format support to the kernel and another fixup to > add NIST P521 support to the in-kernel SPDM library > (two top-most commits on my above-linked development branch). > > I performed this test against your f81547267725 head and notice > that you pushed a new version today (with "curve->nbits == 521" > instead of strcmp), but I'm confident those two small changes > wouldn't alter the outcone, hence my Tested-by stands. > > Thanks, > > Lukas >