Remove temporary allocation in mbedtls_mpi_sub_abs - single subtract implementation #373

2020-02-19T20:39:23-06:00

marchmarch commented

2020-02-19 20:39:23 -06:00

(Migrated from github.com)

As requested in https://github.com/ARMmbed/mbed-crypto/pull/366 here's a new pull request that uses a single subtraction implementation.

mpg commented

2020-03-19 07:04:28 -05:00

(Migrated from github.com)

Hi @marchmarch and sorry for keeping you waiting for so long, I'll finally have time to look seriously into this enhancement you're suggesting (thanks again for that!).

On the legal front, we'll need you to add a Signed-off-by tag to the first two commits in this PR as well, just as you did for the last one. (git rebase -s should do the trick with recent enough git)

Regarding code size, I did some size measurements using arm-none-eabi-gcc -mthumb -mcpu=cortex-m0 -Os -Iinclude -c library/bignum.c && arm-none-eabi-size bignum.o and here what I got:

version	size
merge-base	12771
#366	12851
#373	12791

I also did some performance testing on my x86 machine, looking at the difference between development and this PR, and for FFDH and RSA the different was lower than the measurement noise, while for ECDH and ECDSA there was a measurable improvement.

So I would prefer the version in this PR to the one in #366 - which I understand was also Gilles's preference.

I'll review the code tomorrow.

Hi @marchmarch and sorry for keeping you waiting for so long, I'll finally have time to look seriously into this enhancement you're suggesting (thanks again for that!). On the legal front, we'll need you to add a Signed-off-by tag to the first two commits in this PR as well, just as you did for the last one. (`git rebase -s` should do the trick with recent enough git) Regarding code size, I did some size measurements using `arm-none-eabi-gcc -mthumb -mcpu=cortex-m0 -Os -Iinclude -c library/bignum.c && arm-none-eabi-size bignum.o` and here what I got: | version | size | | --- | --- | | merge-base | 12771 | | #366 | 12851 | | #373 | 12791 | I also did some performance testing on my x86 machine, looking at the difference between development and this PR, and for FFDH and RSA the different was lower than the measurement noise, while for ECDH and ECDSA there was a measurable improvement. So I would prefer the version in this PR to the one in #366 - which I understand was also Gilles's preference. I'll review the code tomorrow.

mpg (Migrated from github.com) reviewed

2020-03-20 04:19:41 -05:00

mpg (Migrated from github.com) left a comment

No description provided.

Looks good to me. I left a suggestion for an improvement, that you can implement if you choose, but the PR is good enough for me as it is now.

marchmarch (Migrated from github.com) reviewed

2020-03-22 21:02:51 -05:00

library/bignum.c

					
				@ -1328,23 +1328,42 @@ cleanup:

				}

				/*

Makes sense. I updated the comment.

marchmarch commented

2020-03-22 21:05:46 -05:00

(Migrated from github.com)

I added signed-off-by to the older commits.

mpg (Migrated from github.com) reviewed

2020-03-23 03:32:37 -05:00

mpg (Migrated from github.com) left a comment

No description provided.

Thanks for adding the comment and the signed-off-by. It all looks good to me now. (The CI is happy too, except for some mbed-os test failing which are due to a know issue independent from this PR.) The next and last step is to get a second member of the team to review (Gilles, who started reviewing the first version of this PR is on leave until April, but perhaps another team member can review), and this will be ready to merge!

ronald-cron-arm (Migrated from github.com) reviewed

2020-03-24 07:39:06 -05:00

gilles-peskine-arm (Migrated from github.com) requested changes

2020-03-24 09:20:19 -05:00

gilles-peskine-arm (Migrated from github.com) left a comment

Correction

Looks good to me apart from one minor issue.

Desirability

On a Cortex-M0+ build with arm-gcc-none-eabi -Os (GCC 7.3.1), bignum.o increases from 15885 bytes to 15909 bytes. This is a negligible increase.

On my PC I see a performance gain (around +10%) with ECDSA/ECDH with P curves, and roughly stable performance otherwise (just with a quick run of the benchmark program).

So this pull request is a nice performance improvement.

### Correction Looks good to me apart from one minor issue. ### Desirability On a Cortex-M0+ build with `arm-gcc-none-eabi -Os` (GCC 7.3.1), `bignum.o` increases from 15885 bytes to 15909 bytes. This is a negligible increase. On my PC I see a performance gain (around +10%) with ECDSA/ECDH with P curves, and roughly stable performance otherwise (just with a quick run of the benchmark program). So this pull request is a nice performance improvement.

library/bignum.c

					
				@ -1328,23 +1328,42 @@ cleanup:

				}

Even though it isn't necessary by C rules, we use the convention that const mbedtls_mpi *X means that the whole bignum X is not modified, i.e. we not only don't modify X->some_field but also we don't modify X->p[some_index] either. Please declare X as non-const here.

Even though it isn't necessary by C rules, we use the convention that `const mbedtls_mpi *X` means that the whole bignum `X` is not modified, i.e. we not only don't modify `X->some_field` but also we don't modify `X->p[some_index]` either. Please declare `X` as non-const here.

marchmarch (Migrated from github.com) reviewed

2020-03-24 20:02:14 -05:00

library/bignum.c

					
				@ -1328,23 +1328,42 @@ cleanup:

				}

I originally had the code that way but mpi_montmul/mpi_montred declare T as const so I get a warning if I don't make X const.

However, it looks like I can remove const from T in both mpi_montmul and mpi_montred without generating any compiler warnings. Is this OK with you?

I originally had the code that way but mpi_montmul/mpi_montred declare T as const so I get a warning if I don't make X const. However, it looks like I can remove const from T in both mpi_montmul and mpi_montred without generating any compiler warnings. Is this OK with you?

gilles-peskine-arm (Migrated from github.com) reviewed

2020-03-25 03:53:57 -05:00

library/bignum.c

					
				@ -1328,23 +1328,42 @@ cleanup:

				}

Yes, please do. I think it was a mistake to make T const there.

Yes, please do. I think it was a mistake to make `T` const there.

marchmarch (Migrated from github.com) reviewed

2020-03-25 06:17:42 -05:00

library/bignum.c

					
				@ -1328,23 +1328,42 @@ cleanup:

				}

OK, I removed const from those 3 functions.

gilles-peskine-arm (Migrated from github.com) approved these changes

2020-03-25 06:42:21 -05:00

mpg (Migrated from github.com) approved these changes

2020-03-25 06:56:52 -05:00

mpg (Migrated from github.com) left a comment

Thanks for making the changes requested by Gilles. The result looks good to me.

mpg commented

2020-03-26 03:01:21 -05:00

(Migrated from github.com)

Everything's passing in the CI except the mbed-os and IAR tests which are known issues independent from this PR.

Sign in to join this conversation.

No reviewers

gilles-peskine-arm

mpg

No labels

HEADER FORMATTING ISSUE

help wanted

Information

invalid

Jira status: AWAITING REVIEW

Jira status: CLOSED

Jira status: DONE

Jira status: IN PROGRESS

Jira status: IN REVIEW

Jira status: OPEN

Jira status: REOPENED

Jira status: RESOLVED

open for community contribution

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-ARMmbed/mbed-crypto!373

				`@ -1328,23 +1328,42 @@ cleanup:`
				`}`

				`@ -1328,23 +1328,42 @@ cleanup:`
				`}`

				`@ -1328,23 +1328,42 @@ cleanup:`
				`}`

				`@ -1328,23 +1328,42 @@ cleanup:`
				`}`