-
Notifications
You must be signed in to change notification settings - Fork 25
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
[ecdsa-sca] Enable ECDSA-256 secret shares for capture #109
Conversation
The C counterpart is here: opentitan-pr16899. |
b964204
to
c681878
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bilgiday for working on this. I've left a few comments and also have some questions.
Please move all the binaries related to ECDSA captures (bitstreams + target binaries) to Git LFS. We should have done this right from the beginning but it seems I wasn't fast enough in reviewing your last/initial ECDSA PR. For this, you'll have to add entries for these binaries in .gitattributes
in the top directory of the repo.
@vogelpi should we add wildcards to the |
c681878
to
0b7b862
Compare
cw/objs/ecc_serial_fpga_cw310.bin filter=lfs diff=lfs merge=lfs -text | ||
cw/objs/ecc384_serial_fpga_cw310.bin filter=lfs diff=lfs merge=lfs -text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think just declaring these files in the .gitattributes
here is enough. As you can see further below in the diff, Git still thinks you've pushed new binary files. You probably have to first delete the files using git rm
, then add them to Git LFS and last but not least squash the commits into one. For reference this is how the diff should look like: https://github.com/lowRISC/ot-sca/pull/4/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed steps. I updated the files by following these instructions.
cw/capture_ecdsa_cw310.yaml
Outdated
# https://github.com/lowRISC/ot-sca/issues/106#issuecomment-1359530023 | ||
# https://rtfm.newae.com/Capture/ChipWhisperer-Husky/#streaming-mode | ||
# A diagram showing the clocking structure for optentitan CW310: | ||
# http://drawings/1oVABuYbeXq5lwneQMZ49D5MBFfBJoSxz6V1A17j4XDo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link doesn't work. What you can do is include it in this PR as .png and then link to that. On the other hand, you don't really refer to this in the file or explain what is meant. So I think it's fine to skip this link in the source file. But it would be worth to add it to the PR for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the link from the source code. Added it to PR (doc/img) for discussion and future reference.
cw/capture_ecdsa_cw310.yaml
Outdated
# Target operating frequency | ||
pll_frequency: 22000000 | ||
# Target operating frequency: | ||
# for ecdsa-simple: 25-50MHz (alignment issues above 66MHZ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: 66MHZ -> 66MHz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing the suggested changes, this looks mostly good now.
What I would like to happen before we merge this:
- @vrozic was planning to also try out the PR on the FPGA
- There still seems to be an issue with Git LFS, I left a more detailed comment.
- It would be good if also @m-temp could have a look at the PR because he will also build on top of this work.
I am not hitting "request changes" because I don't want to block progress (OoO tomorrow) but please make sure to the points above are addressed before merging this.
Good point @m-temp , I don't see a reason for not putting such files into LFS. I just wasn't aware this was possible. Would you mind giving this a try as a follow-up for this PR? |
Ah, and you'll need to rebase this PR on top of master for CI to pass. There was a Python-related issue I had to fix. |
Thanks for the comments @vogelpi. I will fix the Git LFS issue :) |
Thanks for fixing it. I will rebase before pushing the next version. |
2a9a026
to
8fec21a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I've added a few comments that could improve understanding the code in the future. Would be nice to have to address these.
cw/capture.py
Outdated
# Currently, we need to reprogram the firmware before every sign operation | ||
# TODO: Investigate the C code (ecc384_serial) to check if we can run multiple | ||
# signing operations without reloading the firmware. | ||
reset_firmware = True | ||
reset_firmware = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says we need to reprogram, but reset_firmware = False
. Could you explain what's the actual TODO? Is this only a TODO for 384 and set to false for 256?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Michael! It seems that I forgot to update these comments.
At the beginning of the project, there was an issue in the C code, and we needed to reflash the firmware for each signing operation. Now, that problem has been solved and we don't need to reflash the firmware again and again.
I just wanted to keep this reflashing option there in case we need it in the future. I updated the comments accordingly to reflect the current state.
cw/capture.py
Outdated
# Combine the two shares of d and k | ||
priv_key_d = priv_key_d0 + priv_key_d1 | ||
secret_k = secret_k0 + secret_k1 | ||
|
||
# Create an arrey to keep the traces | ||
waves = np.array([]) | ||
# Currently, we have to reload the firmware to be able to send a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this is not port of the current PR could you add a comment, when this branch/condition needs to be taken? See also comment above, where reset_firmware
is set
edit: Hmm there's something wrong with the associated lines. This part belongs tho the reset_firmeware
part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I updated the reset_firmware
related comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well on my setup for 256b version.
Capture rates: ~2.5s per trace (simple mode) and 1.5s per trace (stream mode). I assume these are the expected values.
Thanks for the great work @bilgiday
- Enables sending of all secret shares (k0, k1, d0, d1) from the Python code (capture.py). - Enables the trace collection without needing to reflash the firmware at every iteration. Signed-off-by: Bilgiday Yuce <bilgiday@opentitan.org>
8fec21a
to
b9c113d
Compare
Thanks for testing it @vrozic, the capture rates seems right to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bilgiday for your work and addressing/clarifying the issues!
LGTM
It seems everyone is happy with the current status of the PR. I am merging this PR not to block the others. On the opentitan side, lowRISC/opentitan#16899, I am still waiting for one last nod to go. |
Enable sending of all secret shares (k0, k1, d0, d1) from the Python code (capture.py).
Signed-off-by: Bilgiday Yuce bilgiday@opentitan.org