From: Stefan Berger <[email protected]>
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(-)
--
2.43.0
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
>
> 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 <[email protected]>
> >
> > 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 <[email protected]>
>>
>> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >
> > 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