mbox series

[v6,00/13] Add support for NIST P521 to ecdsa

Message ID 20240312183618.1211745-1-stefanb@linux.vnet.ibm.com (mailing list archive)
Headers show
Series Add support for NIST P521 to ecdsa | expand

Message

Stefan Berger March 12, 2024, 6:36 p.m. UTC
From: Stefan Berger <stefanb@linux.ibm.com>

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.

Regards,
   Stefan

v6:
 - Use existing #defines for number of digits rather than plain numbers
   (1/13, 6/13) following Bharat's suggestion
 - Initialize result from lowest 521 bits of product rather than going
   through tmp variable (6/13)

v5:
 - Simplified ecc_digits_from_bytes as suggested by Lukas (1/12)
 - Using nbits == 521 to detect NIST P521 curve rather than strcmp()
   (5,6/12)
 - Nits in patch description and comments (11/12)

v4:
 - Followed suggestions by Lukas Wummer (1,5,8/12)
 - Use nbits rather than ndigits where needed (8/12)
 - Renaming 'keylen' variablest to bufsize where necessary (9/12)
 - Adjust signature size calculation for NIST P521 (11/12)

v3:
 - Dropped ecdh support
 - Use ecc_get_curve_nbits for getting number of bits in NIST P521 curve
   in ecc_point_mult (7/10)

v2:
 - Reformulated some patch descriptions
 - Fixed issue detected by krobot
 - Some other small changes to the code



Stefan Berger (13):
  crypto: ecc - Use ECC_CURVE_NIST_P192/256/384_DIGITS where possible
  crypto: ecdsa - Convert byte arrays with key coordinates to digits
  crypto: ecdsa - Adjust tests on length of key parameters
  crypto: ecdsa - Extend res.x mod n calculation for NIST P521
  crypto: ecc - Add nbits field to ecc_curve structure
  crypto: ecc - Implement vli_mmod_fast_521 for NIST p521
  crypto: ecc - Add special case for NIST P521 in ecc_point_mult
  crypto: ecc - Add NIST P521 curve parameters
  crypto: ecdsa - Replace ndigits with nbits where precision is needed
  crypto: ecdsa - Rename keylen to bufsize where necessary
  crypto: ecdsa - Register NIST P521 and extend test suite
  crypto: asymmetric_keys - Adjust signature size calculation for NIST
    P521
  crypto: x509 - Add OID for NIST P521 and extend parser for it

 crypto/asymmetric_keys/public_key.c       |  14 ++-
 crypto/asymmetric_keys/x509_cert_parser.c |   3 +
 crypto/ecc.c                              |  44 +++++--
 crypto/ecc_curve_defs.h                   |  49 ++++++++
 crypto/ecdsa.c                            |  62 ++++++---
 crypto/ecrdsa_defs.h                      |   5 +
 crypto/testmgr.c                          |   7 ++
 crypto/testmgr.h                          | 146 ++++++++++++++++++++++
 include/crypto/ecc_curve.h                |   2 +
 include/crypto/ecdh.h                     |   1 +
 include/crypto/internal/ecc.h             |  24 +++-
 include/linux/oid_registry.h              |   1 +
 12 files changed, 335 insertions(+), 23 deletions(-)

Comments

Stefan Berger March 15, 2024, 5:10 p.m. UTC | #1
On 3/12/24 14:36, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> 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.
> 

FYI: A test suite for the NIST ECC algorithms is here: 
https://github.com/stefanberger/eckey-testing

Script to test valid and invalid certs and signatures: 
test-ecc-kernel-keys.sh

> Regards,
>     Stefan
> 
> v6:
>   - Use existing #defines for number of digits rather than plain numbers
>     (1/13, 6/13) following Bharat's suggestion
>   - Initialize result from lowest 521 bits of product rather than going
>     through tmp variable (6/13)
> 
> v5:
>   - Simplified ecc_digits_from_bytes as suggested by Lukas (1/12)
>   - Using nbits == 521 to detect NIST P521 curve rather than strcmp()
>     (5,6/12)
>   - Nits in patch description and comments (11/12)
> 
> v4:
>   - Followed suggestions by Lukas Wummer (1,5,8/12)
>   - Use nbits rather than ndigits where needed (8/12)
>   - Renaming 'keylen' variablest to bufsize where necessary (9/12)
>   - Adjust signature size calculation for NIST P521 (11/12)
> 
> v3:
>   - Dropped ecdh support
>   - Use ecc_get_curve_nbits for getting number of bits in NIST P521 curve
>     in ecc_point_mult (7/10)
> 
> v2:
>   - Reformulated some patch descriptions
>   - Fixed issue detected by krobot
>   - Some other small changes to the code
> 
> 
> 
> Stefan Berger (13):
>    crypto: ecc - Use ECC_CURVE_NIST_P192/256/384_DIGITS where possible
>    crypto: ecdsa - Convert byte arrays with key coordinates to digits
>    crypto: ecdsa - Adjust tests on length of key parameters
>    crypto: ecdsa - Extend res.x mod n calculation for NIST P521
>    crypto: ecc - Add nbits field to ecc_curve structure
>    crypto: ecc - Implement vli_mmod_fast_521 for NIST p521
>    crypto: ecc - Add special case for NIST P521 in ecc_point_mult
>    crypto: ecc - Add NIST P521 curve parameters
>    crypto: ecdsa - Replace ndigits with nbits where precision is needed
>    crypto: ecdsa - Rename keylen to bufsize where necessary
>    crypto: ecdsa - Register NIST P521 and extend test suite
>    crypto: asymmetric_keys - Adjust signature size calculation for NIST
>      P521
>    crypto: x509 - Add OID for NIST P521 and extend parser for it
> 
>   crypto/asymmetric_keys/public_key.c       |  14 ++-
>   crypto/asymmetric_keys/x509_cert_parser.c |   3 +
>   crypto/ecc.c                              |  44 +++++--
>   crypto/ecc_curve_defs.h                   |  49 ++++++++
>   crypto/ecdsa.c                            |  62 ++++++---
>   crypto/ecrdsa_defs.h                      |   5 +
>   crypto/testmgr.c                          |   7 ++
>   crypto/testmgr.h                          | 146 ++++++++++++++++++++++
>   include/crypto/ecc_curve.h                |   2 +
>   include/crypto/ecdh.h                     |   1 +
>   include/crypto/internal/ecc.h             |  24 +++-
>   include/linux/oid_registry.h              |   1 +
>   12 files changed, 335 insertions(+), 23 deletions(-)
>
Lukas Wunner March 18, 2024, 6:48 p.m. UTC | #2
On Tue, Mar 12, 2024 at 02:36:05PM -0400, Stefan Berger wrote:
> This series adds support for the NIST P521 curve to the ecdsa module
> to enable signature verification with it.

v6 of this series is still

Tested-by: Lukas Wunner <lukas@wunner.de>

as it successfully authenticates PCI devices with the
"TPM_ALG_ECDSA_ECC_NIST_P521" SPDM asymmetric algorithm
using my development branch:

https://github.com/l1k/linux/commits/doe

Thanks,

Lukas
Stefan Berger March 18, 2024, 10:42 p.m. UTC | #3
On 3/18/24 14:48, Lukas Wunner wrote:
> On Tue, Mar 12, 2024 at 02:36:05PM -0400, Stefan Berger wrote:
>> This series adds support for the NIST P521 curve to the ecdsa module
>> to enable signature verification with it.
> 
> v6 of this series is still
> 
> Tested-by: Lukas Wunner <lukas@wunner.de>

Thanks.

> 
> as it successfully authenticates PCI devices with the
> "TPM_ALG_ECDSA_ECC_NIST_P521" SPDM asymmetric algorithm
> using my development branch:
> 
> https://github.com/l1k/linux/commits/doe
> 
> Thanks,
> 
> Lukas
>
Jarkko Sakkinen March 19, 2024, 6:22 p.m. UTC | #4
On Tue Mar 19, 2024 at 12:42 AM EET, Stefan Berger wrote:
>
>
> On 3/18/24 14:48, Lukas Wunner wrote:
> > On Tue, Mar 12, 2024 at 02:36:05PM -0400, Stefan Berger wrote:
> >> This series adds support for the NIST P521 curve to the ecdsa module
> >> to enable signature verification with it.
> > 
> > v6 of this series is still
> > 
> > Tested-by: Lukas Wunner <lukas@wunner.de>
>
> Thanks.

This has been discussed before in LKML but generally tested-by for
series does not have semantical meaning.

Please apply only for patches that were tested.

BR, Jarkko
Jarkko Sakkinen March 19, 2024, 6:25 p.m. UTC | #5
On Tue Mar 19, 2024 at 8:22 PM EET, Jarkko Sakkinen wrote:
> On Tue Mar 19, 2024 at 12:42 AM EET, Stefan Berger wrote:
> >
> >
> > On 3/18/24 14:48, Lukas Wunner wrote:
> > > On Tue, Mar 12, 2024 at 02:36:05PM -0400, Stefan Berger wrote:
> > >> This series adds support for the NIST P521 curve to the ecdsa module
> > >> to enable signature verification with it.
> > > 
> > > v6 of this series is still
> > > 
> > > Tested-by: Lukas Wunner <lukas@wunner.de>
> >
> > Thanks.
>
> This has been discussed before in LKML but generally tested-by for
> series does not have semantical meaning.
>
> Please apply only for patches that were tested.

How to implement this in practice or place tested-by correctly?

I'd place the tested-by to the patch which contains the uapi trigger
to which for testing was done.

By having this granularity that also does help fixing bugs later on
so it is really not just "plain bureacracy".

BR, Jarkko
Stefan Berger March 19, 2024, 6:55 p.m. UTC | #6
On 3/19/24 14:22, Jarkko Sakkinen wrote:
> On Tue Mar 19, 2024 at 12:42 AM EET, Stefan Berger wrote:
>>
>>
>> On 3/18/24 14:48, Lukas Wunner wrote:
>>> On Tue, Mar 12, 2024 at 02:36:05PM -0400, Stefan Berger wrote:
>>>> This series adds support for the NIST P521 curve to the ecdsa module
>>>> to enable signature verification with it.
>>>
>>> v6 of this series is still
>>>
>>> Tested-by: Lukas Wunner <lukas@wunner.de>
>>
>> Thanks.
> 
> This has been discussed before in LKML but generally tested-by for
> series does not have semantical meaning.
> 
> Please apply only for patches that were tested.

Ok, I will remove the Tested-by tag.

However, patch 4/13, that only changes a comment, can also be tested in 
so far as to check whether the code is correct as-is for the tests that 
'I' ran and no further modifications are needed for NIST P521. In this 
case it would mean that a single subtraction of 'n' from res.x seems 
sufficient and existing code is good as described by the modified comment.

> 
> BR, Jarkko
>
Jarkko Sakkinen March 19, 2024, 7:14 p.m. UTC | #7
On Tue Mar 19, 2024 at 8:55 PM EET, Stefan Berger wrote:
>
>
> On 3/19/24 14:22, Jarkko Sakkinen wrote:
> > On Tue Mar 19, 2024 at 12:42 AM EET, Stefan Berger wrote:
> >>
> >>
> >> On 3/18/24 14:48, Lukas Wunner wrote:
> >>> On Tue, Mar 12, 2024 at 02:36:05PM -0400, Stefan Berger wrote:
> >>>> This series adds support for the NIST P521 curve to the ecdsa module
> >>>> to enable signature verification with it.
> >>>
> >>> v6 of this series is still
> >>>
> >>> Tested-by: Lukas Wunner <lukas@wunner.de>
> >>
> >> Thanks.
> > 
> > This has been discussed before in LKML but generally tested-by for
> > series does not have semantical meaning.
> > 
> > Please apply only for patches that were tested.
>
> Ok, I will remove the Tested-by tag.
>
> However, patch 4/13, that only changes a comment, can also be tested in 
> so far as to check whether the code is correct as-is for the tests that 
> 'I' ran and no further modifications are needed for NIST P521. In this 
> case it would mean that a single subtraction of 'n' from res.x seems 
> sufficient and existing code is good as described by the modified comment.

So, since all patches are required to test anything at all, I think that
putting tested-by to 13/13 would be the most appropriate, right?

I without enabling this x509 parser, there is nothing to test, I'd
presume.

It doesn't have to be more complicated than this.

BR, Jarkko
Lukas Wunner March 20, 2024, 5:40 a.m. UTC | #8
On Tue, Mar 19, 2024 at 08:22:51PM +0200, Jarkko Sakkinen wrote:
> On Tue Mar 19, 2024 at 12:42 AM EET, Stefan Berger wrote:
> > On 3/18/24 14:48, Lukas Wunner wrote:
> > > On Tue, Mar 12, 2024 at 02:36:05PM -0400, Stefan Berger wrote:
> > >> This series adds support for the NIST P521 curve to the ecdsa module
> > >> to enable signature verification with it.
> > > 
> > > v6 of this series is still
> > > 
> > > Tested-by: Lukas Wunner <lukas@wunner.de>
> >
> > Thanks.
> 
> This has been discussed before in LKML but generally tested-by for
> series does not have semantical meaning.

I believe that notion is outdated.

It seems to be becoming the norm that maintainers apply patches with
"b4 am --apply-cover-trailers", which automatically picks up Acked-by,
Reviewed-by, Tested-by and other tags sent in-reply-to the cover letter
and adds them to all patches in the series.

Consequently, providing such tags in-reply-to the cover letter is not
unusual and nothing to object to.

If Herbert applies patches with "b4 am --apply-cover-trailers" or
"b4 shazam --apply-cover-trailers" (I don't know if he does),
it is completely irrelevant whether Stefan strips my Tested-by from
individual patches:  It will automatically be re-added when the
series gets applied.

I have only tested the collection of *all* patches in this series and
can thus only vouch for correct functioning of the *entire* series,
hence providing the Tested-by in-reply-to the cover letter is the only
thing that makes sense to me.

Either way, I don't think arguing over which patch to apply a Tested-by
to is a productive use of everyone's time.

Thanks,

Lukas
Konstantin Ryabitsev March 20, 2024, 2:41 p.m. UTC | #9
On Wed, Mar 20, 2024 at 06:40:33AM +0100, Lukas Wunner wrote:
> If Herbert applies patches with "b4 am --apply-cover-trailers" or
> "b4 shazam --apply-cover-trailers" (I don't know if he does),
> it is completely irrelevant whether Stefan strips my Tested-by from
> individual patches:  It will automatically be re-added when the
> series gets applied.

Applying trailers sent to the cover letter is now the default behaviour in
0.13, so this flag is no longer required (it does nothing).

-K
Jarkko Sakkinen March 21, 2024, 4:17 p.m. UTC | #10
On Wed Mar 20, 2024 at 4:41 PM EET, Konstantin Ryabitsev wrote:
> On Wed, Mar 20, 2024 at 06:40:33AM +0100, Lukas Wunner wrote:
> > If Herbert applies patches with "b4 am --apply-cover-trailers" or
> > "b4 shazam --apply-cover-trailers" (I don't know if he does),
> > it is completely irrelevant whether Stefan strips my Tested-by from
> > individual patches:  It will automatically be re-added when the
> > series gets applied.
>
> Applying trailers sent to the cover letter is now the default behaviour in
> 0.13, so this flag is no longer required (it does nothing).
>
> -K

The whole policy of how to put tested-by in my experience is subsystem
dependent.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Official documentation only speaks about patches so perhaps it should
then be refined for the series.

I'm hearing about this option in b4 for the first time in my life.

BR, Jarkko
Jarkko Sakkinen March 21, 2024, 4:19 p.m. UTC | #11
On Thu Mar 21, 2024 at 6:17 PM EET, Jarkko Sakkinen wrote:
> On Wed Mar 20, 2024 at 4:41 PM EET, Konstantin Ryabitsev wrote:
> > On Wed, Mar 20, 2024 at 06:40:33AM +0100, Lukas Wunner wrote:
> > > If Herbert applies patches with "b4 am --apply-cover-trailers" or
> > > "b4 shazam --apply-cover-trailers" (I don't know if he does),
> > > it is completely irrelevant whether Stefan strips my Tested-by from
> > > individual patches:  It will automatically be re-added when the
> > > series gets applied.
> >
> > Applying trailers sent to the cover letter is now the default behaviour in
> > 0.13, so this flag is no longer required (it does nothing).
> >
> > -K
>
> The whole policy of how to put tested-by in my experience is subsystem
> dependent.
>
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
> Official documentation only speaks about patches so perhaps it should
> then be refined for the series.
>
> I'm hearing about this option in b4 for the first time in my life.

It is also pretty relevant to know when you read the commit log e.g.
when bisecting what was *actually* tested.

If you put tested-by to whole series it probably means that you've
tested the uapi and are getting the expected results. Thus in this
case it would 13/13 that is *actually* tested.

Putting tested-by to every possible patch only degrades the quality
of the commit log.

I don't see how this is "irrelevant".

BR, Jarkko
Stefan Berger March 21, 2024, 4:36 p.m. UTC | #12
On 3/21/24 12:19, Jarkko Sakkinen wrote:
> On Thu Mar 21, 2024 at 6:17 PM EET, Jarkko Sakkinen wrote:
>> On Wed Mar 20, 2024 at 4:41 PM EET, Konstantin Ryabitsev wrote:
>>> On Wed, Mar 20, 2024 at 06:40:33AM +0100, Lukas Wunner wrote:
>>>> If Herbert applies patches with "b4 am --apply-cover-trailers" or
>>>> "b4 shazam --apply-cover-trailers" (I don't know if he does),
>>>> it is completely irrelevant whether Stefan strips my Tested-by from
>>>> individual patches:  It will automatically be re-added when the
>>>> series gets applied.
>>>
>>> Applying trailers sent to the cover letter is now the default behaviour in
>>> 0.13, so this flag is no longer required (it does nothing).
>>>
>>> -K
>>
>> The whole policy of how to put tested-by in my experience is subsystem
>> dependent.
>>
>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>>
>> Official documentation only speaks about patches so perhaps it should
>> then be refined for the series.
>>
>> I'm hearing about this option in b4 for the first time in my life.
> 
> It is also pretty relevant to know when you read the commit log e.g.
> when bisecting what was *actually* tested.
> 
> If you put tested-by to whole series it probably means that you've
> tested the uapi and are getting the expected results. Thus in this
> case it would 13/13 that is *actually* tested.

Btw, we have 2 entry points into the code and those are uapi and testmgr.

So if I was to exercise the uapi with a NIST P521 key then are you 
saying that none of the other code was exercised and therefore wasn't 
tested? How would YOU suggest to test individual patches then?

What the docs at the link above say is this:

A Tested-by: tag indicates that the patch has been successfully tested 
(in some environment) by the person named. This tag informs maintainers 
that some testing has been performed, provides a means to locate testers 
for future patches, and ensures credit for the testers.

Note: 'some testing' 'in some environment'. We probably can reasonably 
assume that not only 13/13 is necessary but also several of the other 
patches are necessary to support this new curve and were exercised with 
either UAPI and probably also testmgr.

> 
> Putting tested-by to every possible patch only degrades the quality
> of the commit log.

I would still be interested how one would test individual patches in a 
series so they are worthy of a Tested-by tag.

> 
> I don't see how this is "irrelevant".
> 
> BR, Jarkko
Jarkko Sakkinen March 21, 2024, 4:50 p.m. UTC | #13
On Thu Mar 21, 2024 at 6:36 PM EET, Stefan Berger wrote:
> > 
> > Putting tested-by to every possible patch only degrades the quality
> > of the commit log.
>
> I would still be interested how one would test individual patches in a 
> series so they are worthy of a Tested-by tag.

I've at least said this twice in this thread.

I.e. in a feature you most likely test the uapi so 13/13.

In a bug fix you test kernel with and without the patch. Generally you
test stuff that you can observe.

You can also test non-uapi behaviour with e.g. kprobes or measure
e.g. performance, depending on patch.

BR, Jarkko