Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sca/WIP] Update the ecdsa384_sca code #19380

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

bilgiday
Copy link
Contributor

@bilgiday bilgiday commented Aug 7, 2023

Update ecc384_serial.c and p384_ecdsa_sca.s files to be compatible with the latest capture.py file in the ot-sca repo.

This is an WIP PR.

@bilgiday
Copy link
Contributor Author

bilgiday commented Aug 7, 2023

@wettermo, @jadephilipoom,

I modified these two files based on the p256_serial.c file in th OT-repo, and the p384_ecdsa_sign_test.s file in the PR #19290.

I was able to compile the files using #19290. @wettermo, you can use them in your local environment and test if they work with the ot-sca tooling.

@wettermo
Copy link
Contributor

wettermo commented Aug 8, 2023

Thanks a lot @bilgiday ! I'll keep you updated on my progress :) .

Copy link
Contributor

@wettermo wettermo left a comment

Choose a reason for hiding this comment

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

LGTM! I tested the changes on my PC/FPGA setup: no errors and compatible with capture.py in ot-sca repository.
Thanks a lot @bilgiday !!

@bilgiday bilgiday force-pushed the ecc384_sca_fix branch 2 times, most recently from ab0a033 to 9d96fbe Compare August 9, 2023 13:26
@bilgiday
Copy link
Contributor Author

bilgiday commented Aug 9, 2023

@wettermo Thanks for checking it!

@jadephilipoom, It looks like the CI checks also pass now. If it looks okay to you, we can merge this. WDYT?

@bilgiday bilgiday marked this pull request as ready for review August 10, 2023 08:40
@bilgiday bilgiday requested a review from a team as a code owner August 10, 2023 08:40
@vrozic
Copy link
Contributor

vrozic commented Aug 10, 2023

Thanks @bilgiday for working on this. The code looks good to me, modulo comment from @wettermo.

We can update the binaries in ot-sca once this is merged. For now, I've opened ot-sca Issue 149 so we don't forget.

@bilgiday
Copy link
Contributor Author

@vrozic Great, thanks for opening the ot-sca-issue. I will update the binaries after this PR is merged.

@moidx moidx removed the request for review from a team August 11, 2023 21:18
@vogelpi
Copy link
Contributor

vogelpi commented Sep 8, 2023

This PR is a bit older than a month an in attempt to move things forward, I've taken the liberty to rebase this on current master.

Update ecc384_serial.c and p384_ecdsa_sca.s files to be compatible with
the latest capture.py file in the ot-sca repo.

Signed-off-by: Bilgiday Yuce <bilgiday@google.com>
@vogelpi
Copy link
Contributor

vogelpi commented Sep 8, 2023

Update: I've now also included the change suggested by @wettermo and fixed some more comments referring to P256 as well as a missing factor of two (was missing already before). I will now approve this PR and merge it once CI passes.

Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for updating these apps @bilgiday !

Copy link
Contributor

@jadephilipoom jadephilipoom left a comment

Choose a reason for hiding this comment

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

Belated LGTM from me 🙂

@jadephilipoom jadephilipoom merged commit 2ec335c into lowRISC:master Sep 12, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants