Closed Bug 1751705 Opened 3 years ago Closed 3 years ago

Update ECCKiila generated implementations of secp521r1 and secp384r1

Categories

(NSS :: Libraries, enhancement, P1)

Product:
NSS
Component:
Libraries
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: iaroslav.gridin, Assigned: iaroslav.gridin)

Details

(Whiteboard: [nss-fx])

Attachments

(1 file, 1 obsolete file)

Bug 1751705 - Update ECCKiila generated files
48 bytes, text/x-phabricator-request
Details | Review

Actual results:

Implementations are out of date

Bernstein-Yang constant-time field inversion for P-384, replacing FLT
Explicitly initialize curve point data structures
Various fiat-crypto upstream changes

Assignee: nobody → iaroslav.gridin
Severity: -- → N/A
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Whiteboard: [nss-fx]

Hi Iaroslav, thanks for updating that code, could you add a benchmark of both old and new versions of the code here, please? TY!

Flags: needinfo?(iaroslav.gridin)

Ben, would we like to have intel intrinsics code instead of their uint128 addition and subtraction?

Flags: needinfo?(bbeurdouche)

(In reply to Benjamin Beurdouche [:beurdouche] from comment #2)

Hi Iaroslav, thanks for updating that code, could you add a benchmark of both old and new versions of the code here, please? TY!

I ran ecperf with both versions. Any other specific benchmark that could be useful?

Updated code
Testing NIST-P384 using freebl implementation...
    ECDH_Derive     count:100000 sec: 168.96 op/sec: 591.86
    ECDSA_Sign      count:100000 sec: 33.09 op/sec: 3021.97
        ECDHE max rate = 903.46
    ECDSA_Verify    count:100000 sec: 99.94 op/sec: 1000.65
... okay.


Testing NIST-P521 using freebl implementation...
    ECDH_Derive     count:100000 sec: 89.69 op/sec: 1114.89
    ECDSA_Sign      count:100000 sec: 31.51 op/sec: 3173.39
        ECDHE max rate = 1072.07
    ECDSA_Verify    count:100000 sec: 62.48 op/sec: 1600.39
... okay.

current code
Testing NIST-P384 using freebl implementation...
    ECDH_Derive     count:100000 sec: 170.91 op/sec: 585.09
    ECDSA_Sign      count:100000 sec: 33.76 op/sec: 2961.80
        ECDHE max rate = 886.72
    ECDSA_Verify    count:100000 sec: 101.02 op/sec: 989.92
... okay.

Testing NIST-P521 using freebl implementation...
    ECDH_Derive     count:100000 sec: 89.08 op/sec: 1122.58
    ECDSA_Sign      count:100000 sec: 31.33 op/sec: 3191.73
        ECDHE max rate = 1078.58
    ECDSA_Verify    count:100000 sec: 61.46 op/sec: 1627.07
... okay.
Flags: needinfo?(iaroslav.gridin)

This looks perfectly fine, no other benchmark needed : ) Thank you Iaroslav !

Flags: needinfo?(bbeurdouche)

Awesome! Thanks a lot!

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Assignee: nobody → iaroslav.gridin
Status: NEW → ASSIGNED
Flags: needinfo?(bbeurdouche)

I have a small idea:

For HACL* code we have a docker that checks that the code is the same as in the hacl* master. Would we be interested in having the same for ECCKiila?

As the first step, when you have updated the code, where does it come from? Is it somewhere in github?

The code is generated by ECCKiila templates https://gitlab.com/nisec/ecckiila/-/blob/master/README.md, so a way to do this

...would be to incorporate the ECCKiila itself in some way.

Hi,

why do you have two patches? They seem to be changing the same files...

Flags: needinfo?(iaroslav.gridin)

Not sure how the second one got submitted, but now I've updated the D135765 and that one should be merged (couldn't find a way to delete the other one from Phabricator)

Flags: needinfo?(iaroslav.gridin)

You have "Abandon the revision" (or something like this) in the action panel (right after the code changes).

Attachment #9260381 - Attachment is obsolete: true

(In reply to Iaroslav Gridin from comment #12)

...would be to incorporate the ECCKiila itself in some way.

I think this could be a nice thing to have (if one wishes to implement that) <- Leaving for future.

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:iaroslav.gridin, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Flags: needinfo?(nkulatova)
Flags: needinfo?(iaroslav.gridin)

Landing.

Flags: needinfo?(nkulatova)

Thank you. I will also make the docker image later.

Flags: needinfo?(iaroslav.gridin)

(In reply to Iaroslav Gridin from comment #19)

Thank you. I will also make the docker image later.

Ok, thanks :)

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: