[sw,otbnsim] Replace ignored subfunction by nops #28592
Conversation
h-filali
force-pushed
the
otbn-sim-nop-ignored-subfuncs
branch
from
e46744a to
25c22c4
Compare
rswarbrick
left a comment
rswarbrick
left a comment
There was a problem hiding this comment.
One nitty suggestion, but I think I don't properly understand how this works, so can't really give it a proper review yet.
I'd suggest:
- Add a commit message that explains why a change is needed ("with the previous code XYZ reported a spurious error because ABC. This change avoids the problem")
- Also explain how the
control_deps_ignorestuff incheck_const_timewas supposed to work in the past and why it can be ripped out. - In the code itself, I think I might need a few more comments to make sure I understand what's going on. Squinting a little, I think it might be possible to make simpler, but I'm not sure I understand how yet!
johannheyszl
left a comment
johannheyszl
left a comment
There was a problem hiding this comment.
I am not familiar enough with this part to review properly, but the intent to exclude failing const time check in cases of caused by FI makes sense. Thanks @h-filali
This allows subfunctions to be replaced by NOPs for constant time tests. This is useful for the case where a function runs in constant time under normal conditions but is not necessarily constant time in case of FI. Co-authored-by: Rupert Swarbrick <[email protected]> Signed-off-by: Hakim Filali <[email protected]>
h-filali
force-pushed
the
otbn-sim-nop-ignored-subfuncs
branch
from
25c22c4 to
4795b06
Compare
|
Thanks @rswarbrick for your helpful review. I implemented your suggested changes. I also added you as a co-author since I used a lot of your code to improve this PR. Also thanks @johannheyszl for your review. |
vogelpi
left a comment
vogelpi
left a comment
There was a problem hiding this comment.
Thanks @h-filali and @rswarbrick , this looks good to me and we really need this for another PR but I would like @rswarbrick to have the final word.
h-filali
added
the
CherryPick:earlgrey_1.0.0
rswarbrick
left a comment
rswarbrick
left a comment
There was a problem hiding this comment.
Looks good to me, and thanks for the tweaks.
|
Successfully created backport PR for |
4 participants
This allows subfunctions to be replaced by NOPs for constant time tests.
This is useful for the case where a function runs in constant time under normal conditions but is not
necessarily constant time in case of FI.
The issue I was facing was that we have constant time tests in the CI which check whether assembly functions have a constant execution time. For example p384_isoncurve_check is a function that should, under normal conditions, execute in constant time. However it calls
trigger_fault_if_fg0_not_zwhich is not constant time in the case of fault injection. This trips up the consttime check, since all of a sudden it is possible to terminate earlier.For this reason this PR makes it possible to replace the function body of
trigger_fault_if_fg0_not_zby NOPs. This way the constant time can still check the rest ofp384_isoncurve_checkfor constant-time-ness.