-
Notifications
You must be signed in to change notification settings - Fork 26
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
[OTBN] vertical attack initial work #115
Conversation
bed7225
to
df6da38
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.
Hi @m-temp ,
thanks for the PR!
I had a first brief look now. I think we should try to keep this capture setup as generic as possible because we're going to use it for other code snippets than keygen as well. Also, since the captures are short we don't need some of the infrastructure used in for ECDSA captures. The whole capture and analysis is much more similar to KMAC/AES. Maybe it's best to discuss this tomorrow, I've set up a little sync meeting.
85bb10b
to
652ce81
Compare
I've added a first fixed-vs-random tvla version.
|
f48d61f
to
2e51a0a
Compare
9f40120
to
63ec147
Compare
63ec147
to
049f5f4
Compare
To expedite cooperation, I mark this PR as ready for review It adds:
A brief test confirmed that sha3, kmac, aes are not broken. What is still missing:
|
da4df2c
to
0f50aca
Compare
7d3ea70
to
f80f968
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.
Hello! Thanks for the PR. Sorry, I don't have any hardware to try it out for now. I think, in general it looks good. I've added a couple of feedbacks.
Thanks @abdullahvarici for your review and sry for all those typos in the code. I will address them. |
26dff15
to
9cb2d3c
Compare
9cb2d3c
to
e813226
Compare
e813226
to
cfcd269
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.
Overall this looks good and we should move forward. Many thanks for all the work you've put into this @m-temp and @wettermo .
In addition to the OTBN vertical support, this PR contains a few very nice additions that should be factored out into separate PRs. These additions are uncontroversial and are unrelated to OTBN vertical support.
cw/capture.py
Outdated
ot.scope.adc.bits_per_sample = 12 | ||
ot.scope.adc.samples = capture_cfg["num_samples"] | ||
else: | ||
# TODO: Add cw-lite support |
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.
We won't add CW-Lite support. There is actually an issue to remove CW-Lite support ;-)
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.
@vogelpi, should we then remove the TODO
with the cleaning PR proposed in this comment above? probably, keeping the else statement and the message is still OK.
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.
Hm, I am not sure I understand your comment. I think we don't want to have a TODO to Add CW-Lite support, because we're never going to add it but to error out if CW-Lite is detected is the best thing to do here.
cw/tvla.py
Outdated
traces[i_trace] = project.waves[i_trace + | ||
trace_start][sample_start:sample_start + | ||
num_samples] | ||
else: # FIXME: The wave slicing is only tested for otbn |
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 is a very nice feature. Thanks for adding this!
It will also be very useful for other targets. It's not expected to test this for all different targets (there are simply too many options and we have other priorities). However, I would appreciate if you changed the code such that there is simply an assertion to check that when sample_start and sample_end are set, also otbn is selected and provide a comment where this check is done. This way we have to change much less code later on when enabling it e.g. for AES or KMAC.
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.
wilco
cw/tvla.py
Outdated
xaxs = range(sample_start, sample_start + num_samples) | ||
fig, axs = plt.subplots(3, sharex=True) | ||
axs[0].plot(single_trace, "k") | ||
axs[0].set_title( | ||
"TVLA of " + cfg["project_file"] + '\n' + "No. of traces: " + str(num_traces)) | ||
axs[0].plot(xaxs, single_trace, "k") | ||
axs[0].set_ylabel("trace") | ||
for i_order in range(num_orders): | ||
axs[1 + i_order].plot(ttest_trace[i_order, 0, 0], "k") | ||
axs[1 + i_order].plot(c * threshold, "r") | ||
axs[1 + i_order].plot(-threshold * c, "r") | ||
axs[1 + i_order].plot(xaxs, ttest_trace[i_order, 0, 0], "k") | ||
axs[1 + i_order].plot(xaxs, c * threshold, "r") | ||
axs[1 + i_order].plot(xaxs, -threshold * c, "r") |
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 commit needs to be merged with the one adding support for sample_start and num_samples. Also these two things should go into a separate PR.
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.
wilco
cw/tvla.py
Outdated
axs[0].set_title( | ||
"TVLA of " + cfg["project_file"] + '\n' + "No. of traces: " + str(num_traces)) |
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 think just this line here actually belongs into this very commit.
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.
Hmm let's put it also in the other PR, where we change the plotting
Also updating the bitstreams and existing binaries can be factored out. Remember the goal is always to have as small PRs and commit as possible. This speeds up the review and allows us to progress faster as a team. |
94dcfd1
to
a0195f5
Compare
Thx all for the reviews. I changed it accordingly and will file a couple of PRs for the remaining features. |
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 addressing the comments. This looks good!
Two minor nits:
- There is still a TODO for the CW-Lite, but we won't add support for this. Having the script to error out for something else than Husky still desirable though.
- The PR also adds a new bitstream in the first commit. Is this really needed? We wanted to add the new bistream with the different URND behavior in a separate PR I believe.
cw/capture.py
Outdated
ot.scope.adc.bits_per_sample = 12 | ||
ot.scope.adc.samples = capture_cfg["num_samples"] | ||
else: | ||
# TODO: Add cw-lite support |
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.
Hm, I am not sure I understand your comment. I think we don't want to have a TODO to Add CW-Lite support, because we're never going to add it but to error out if CW-Lite is detected is the best thing to do here.
Sry. I missed that. This was a leftover from Jade. I'll remove the comment, but keep the if-else-branch
Ah okay, I thought this was only related to the second bins, but you are right, I delete the old bins and file a separate PR for the new ones. |
a0195f5
to
b5469cb
Compare
Co-authored-by: Jade Philipoom <jadep@google.com> Signed-off-by: Michael Tempelmeier <michael.tempelmeier@gi-de.com>
Signed-off-by: Michael Tempelmeier <michael.tempelmeier@gi-de.com>
This is a initial work to add fixed-vs.-random tvla tests for otbn. Signed-off-by: Michael Tempelmeier <michael.tempelmeier@gi-de.com>
Signed-off-by: Michael Tempelmeier <michael.tempelmeier@gi-de.com>
b5469cb
to
ab9d22a
Compare
This is a PR to bring together the works of Jade, Bilgiday and me
Co-authored-by: Jade Philipoom jadep@google.com
Signed-off-by: Michael Tempelmeier michael.tempelmeier@gi-de.com
Things to do:
[ ] Restructure code to support different code snippets[ ] add select function to simple serial (python + c)[ ] put "golden model" in a function to support different calculations