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