apps/speed: fix weird EC arrays usage by FdaSilvaYY · Pull Request #6133 · openssl/openssl

FdaSilvaYY · 2018-04-29T23:20:32Z

because there is actually 18 curves defined, but only 16 are plugged for ecdsa test
Deduce directly array size using OSSL_NELEM and so remove various magic numbers.
Require some declarations moving.
Implement OPT_PAIR list search without a null-ending element.

IMHO, this PR deserves a backport to 1.1.0

Checklist

documentation is added or updated
tests are added or updated

FdaSilvaYY · 2018-04-29T23:27:05Z

apps/speed.c

Useless indirection through retval

FdaSilvaYY · 2018-04-29T23:34:57Z

apps/speed.c

was an useless indirection through retval

FdaSilvaYY · 2018-04-29T23:35:40Z

apps/speed.c

Hopefully, there is no OOB because of the values that are used

FdaSilvaYY · 2018-04-29T23:36:13Z

apps/speed.c

two last elements were not initialized and used

kroeckx · 2018-04-30T10:06:06Z

apps/speed.c

Any reason you rename i to ix?

Fixed, I just change the signature of found() method, and it is less intrusive.

FdaSilvaYY · 2018-04-30T10:25:56Z

Hi, Renamed in a few locations (not all) just to shutoff a signed/unsigned pointer cast warning when calling 'found' method. Feel free to suggest a better naming.

…

Le 30 avr. 2018 à 12:06, Kurt Roeckx ***@***.***> a écrit : @kroeckx commented on this pull request. In apps/speed.c: > const char *engine_id = NULL; const EVP_CIPHER *evp_cipher = NULL; double d = 0.0; OPTION_CHOICE o; int multiblock = 0, pr_header = 0; int doit[ALGOR_NUM] = { 0 }; - int ret = 1, i, k, misalign = 0; + int ret = 1, ix, misalign = 0; Any reason you rename i to ix? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

dot-asm · 2018-05-01T13:31:24Z

apps/speed.c

I wonder what would people think of following pattern

static const char *names[] = { #define D_MD2 0 "md2", #define D_MDC2 1 "mdc2", ...

It's still ugly, but at least it gives you right idea about relationship between names and indices....

Other suggestion :

enum { D_MD2, D_MDC2, D_MD4, ... static const char *names[] = { "md2", /* D_MD2 */ "mdc2", /* D_MDC2 */ "md4", /* D_MD4 */ ...

Shorter and more speaking form and format, IMHO

Another suggestion (if this is really important) is to have an enum and then a "pair" typedef that has value/string-name. We do that in many other places.

I add these suggestions to the many more ideas I have to simplify and document this code.

dot-asm · 2018-05-01T14:08:59Z

It appears that it would be dependent on #6132 in sense that it would require re-base, so that final review would have to be postponed...

FdaSilvaYY · 2018-05-01T14:21:13Z

This could possibly have a merge issue with #6132, but it will be easy to fixed.
I will rebased as soon I can , once any of two PR is merged.

bernd-edlinger · 2018-05-04T18:59:47Z

Aehm, I would need some performance numbers for brainpool vs. nist vs. Ed25519
does this PR provide such numbers?

dot-asm · 2018-05-09T09:33:35Z

apps/speed.c

If additional empty line is meant to delimit prime and 2m fields, then how come there is no one prior brainpool chunk?

dot-asm · 2018-05-09T09:46:33Z

apps/speed.c

Hmm, this is not right. Basically the difference between these values is approximate difference between speeds, so I'd suggest count / 8000 for X448.

The brp numbers are unreasonable too. The brp256 should be ~3-5 smaller than they are, then loop condition is totally wrong for chosen indices. Indeed, the R and T are interleaved, so it should be i+=2, and it should be <= R_EC_BRP512[RT]1...

I change my mind twice about interleaving or not : that's why this change is awkward.
You know that this juggle around estimating iteration counts is mostly useless, because SIGALRM define and support is the main use-case ...

If you interleave, then it's i+=2, if you ++i, then there should be no interleave. If argument is that it's ~~not~~ non-main use case, then it's argument in favour of whole thing removal, not for suggesting code that is visibly (and verifiably) wrong. There is no excuse for that.

dot-asm · 2018-05-09T10:04:11Z

apps/speed.c

Why? Original condition was correct.

dot-asm · 2018-05-10T09:08:38Z

apps/speed.c

Once again, <=, not <.

dot-asm · 2018-05-10T09:19:27Z

apps/speed.c

Suggestion was that x448 coefficient would be 8 times larger than x25519. I.e. if X25519 was 1000, then X448 would be 8000. Slower algorithm -> higher coefficient. X448 is slower than X25519, 8 times (on x86_64). Well, one can argue that it's not really fair comparison, and it should be approximately square of key lengths' ratio, all right, let's say 4. So if you choose 2200 for X25519, then it should be 8800 for X448.

I made some measurements of X25519/X499 against ecdhb163 to deduce these ratios, but I inverse my calculation Oups ;(
I get a x3 ratio on my computer between X25519 and X499, but ok for a x4 ratio.

because there is actually 18 curves defined, but only 16 are plugged for ecdsa test Deduce directly array size using OSSL_NELEM and so remove various magic numbers. Require some declarations moving. Implement OPT_PAIR list search without a null-ending element. Fix some comparison between signed and unsigned integer expressions.

dot-asm · 2018-05-11T08:58:36Z

As for 1.1.0, does it cherry-pick? [No, I didn't test.]

because there are actually 18 curves defined, but only 16 are plugged for ecdsa test. Deduce array size using OSSL_NELEM and so remove various magic numbers, which required some declarations moving. Implement OPT_PAIR list search without a null-ending element. Fix some comparison between signed and unsigned integer expressions. Reviewed-by: Andy Polyakov <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #6133)

Reviewed-by: Andy Polyakov <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #6133)

dot-asm · 2018-05-12T18:20:20Z

Merged. Thanks!

because there are actually 17 curves defined, but only 16 are plugged for ecdsa test. Deduce array size using OSSL_NELEM and so remove various magic numbers, which required some declarations moving. Implement OPT_PAIR list search without a null-ending element. Fix some comparison between signed and unsigned integer expressions. cherry-picking from commit 5c6a69f. Partial Back-port of openssl#6133 to 1.1.0

because there are actually 17 curves defined, but only 16 are plugged for ecdsa test. Deduce array size using OSSL_NELEM and so remove various magic numbers, which required some declarations moving. Implement OPT_PAIR list search without a null-ending element. Fix some comparison between signed and unsigned integer expressions. cherry-picking from commit 5c6a69f. Partial Back-port of #6133 to 1.1.0 Reviewed-by: Andy Polyakov <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #6245)

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch from 6fe79d9 to 9983953 Compare April 29, 2018 23:20

FdaSilvaYY changed the title ~~app/speed: fix OOB access in some EC arrays~~ app/speed: fix weird EC arrays uses Apr 29, 2018

FdaSilvaYY commented Apr 29, 2018

View reviewed changes

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch from 9983953 to 2d7e6a5 Compare April 29, 2018 23:38

FdaSilvaYY mentioned this pull request Apr 29, 2018

apps/speed: merge curve list definitions #6132

Closed

FdaSilvaYY changed the title ~~app/speed: fix weird EC arrays uses~~ apps/speed: fix weird EC arrays uses Apr 29, 2018

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch 6 times, most recently from 249481a to 3e149f7 Compare April 30, 2018 00:37

FdaSilvaYY changed the title ~~apps/speed: fix weird EC arrays uses~~ WIP: apps/speed: fix weird EC arrays uses Apr 30, 2018

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch 3 times, most recently from 680340e to f2c2343 Compare April 30, 2018 10:00

kroeckx reviewed Apr 30, 2018

View reviewed changes

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch from f2c2343 to b67296a Compare April 30, 2018 10:09

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch 3 times, most recently from a171db0 to 63f0afe Compare April 30, 2018 14:15

FdaSilvaYY changed the title ~~WIP: apps/speed: fix weird EC arrays uses~~ apps/speed: fix weird EC arrays uses Apr 30, 2018

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch from 63f0afe to 83a4d32 Compare April 30, 2018 21:40

dot-asm reviewed May 1, 2018

View reviewed changes

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch 2 times, most recently from 5096d32 to 4a648c3 Compare May 3, 2018 20:11

dot-asm reviewed May 9, 2018

View reviewed changes

apps/speed.c Outdated

Copy link

Contributor

dot-asm May 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Original condition was correct.

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch from db44e20 to f389005 Compare May 9, 2018 20:58

FdaSilvaYY changed the title ~~apps/speed: fix weird EC arrays uses~~ apps/speed: fix weird EC arrays usage May 9, 2018

dot-asm reviewed May 10, 2018

View reviewed changes

apps/speed.c Outdated

Copy link

Contributor

dot-asm May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again, <=, not <.

dot-asm reviewed May 10, 2018

View reviewed changes

FdaSilvaYY added 2 commits May 10, 2018 13:33

apps/speed: Add brainpool curves support

07ee0ad

FdaSilvaYY force-pushed the app/speed/fix-ec-num-issue branch from f389005 to 07ee0ad Compare May 10, 2018 11:34

dot-asm approved these changes May 11, 2018

View reviewed changes

dot-asm added the approval: review pending This pull request needs review by a committer label May 11, 2018

richsalz approved these changes May 11, 2018

View reviewed changes

richsalz added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels May 11, 2018

levitte pushed a commit that referenced this pull request May 12, 2018

apps/speed: Add brainpool curves support

1c53456

Reviewed-by: Andy Polyakov <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #6133)

dot-asm closed this May 12, 2018

FdaSilvaYY mentioned this pull request May 13, 2018

apps/speed: fix possible OOB access in some EC arrays (1.1.0) #6245

Closed

2 tasks

FdaSilvaYY deleted the app/speed/fix-ec-num-issue branch July 19, 2018 20:37

FdaSilvaYY mentioned this pull request Sep 5, 2018

Support EdDSA in apps/speed #7073

Closed

1 task

InfoHunter mentioned this pull request Feb 15, 2019

apps/speed: fix segfault while looking up algorithm name #8244

Closed

Uh oh!

Comments

Conversation

FdaSilvaYY commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

FdaSilvaYY Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY commented Apr 30, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dot-asm commented May 1, 2018

Uh oh!

FdaSilvaYY commented May 1, 2018

Uh oh!

bernd-edlinger commented May 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dot-asm May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dot-asm commented May 11, 2018

Uh oh!

dot-asm commented May 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FdaSilvaYY commented Apr 29, 2018 •

edited

Loading

FdaSilvaYY Apr 29, 2018 •

edited

Loading

FdaSilvaYY Apr 29, 2018 •

edited

Loading

FdaSilvaYY Apr 29, 2018 •

edited

Loading

FdaSilvaYY Apr 30, 2018 •

edited

Loading

FdaSilvaYY May 1, 2018 •

edited

Loading

dot-asm May 9, 2018 •

edited

Loading