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

[hmac, rtl] Do not skip padding after a hash stop command #25590

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrea-caforio
Copy link
Contributor

This commit removes some conditional FSM states transition that were deemed undesirable in prim_sha2_pad, see #23936. It made it possible to cancel an ongoing padding operation, which is disallowed by the HMAC module. As a side-effect, the removal plugs some existing FSM transition coverage holes in prim_sha2_pad whose manual exclusions can be removed as well.

This change makes it impossible to transition into the StFifoReceive state from either StPad80, StPad00, StLenHi or StLenLo by asserting the hash_go signal.

@andrea-caforio andrea-caforio self-assigned this Dec 10, 2024
@andrea-caforio andrea-caforio force-pushed the hmac-disallow-padding-cancellation branch from 2e5c716 to f2b230b Compare December 11, 2024 08:38
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

Splatting the exclusion list might be right (I haven't reviewed it carefully, but it seems reasonable). But doesn't the RTL change also imply that the UNR list might change?

@andrea-caforio andrea-caforio added the CI:Rerun Rerun failed CI jobs label Dec 12, 2024
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Dec 12, 2024
@andrea-caforio
Copy link
Contributor Author

Splatting the exclusion list might be right (I haven't reviewed it carefully, but it seems reasonable). But doesn't the RTL change also imply that the UNR list might change?

Thank you for the review @rswarbrick. You're right, the UNR file should be updated too. Unfortunately, we cannot generate these exclusion files with our VCS license on the Zurich workstation.

@andrea-caforio andrea-caforio marked this pull request as ready for review December 12, 2024 09:54
@andrea-caforio andrea-caforio requested a review from a team as a code owner December 12, 2024 09:54
@andrea-caforio andrea-caforio requested review from eshapira and removed request for a team December 12, 2024 09:54
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.

This looks good to me from an RTL perspective. Thanks for the PR @andrea-caforio !

Did you run a full regression for this and inspect the coverage yet?

@andrea-caforio andrea-caforio force-pushed the hmac-disallow-padding-cancellation branch from f2b230b to 8d79a9b Compare December 18, 2024 14:42
@andrea-caforio
Copy link
Contributor Author

This looks good to me from an RTL perspective. Thanks for the PR @andrea-caforio !

Did you run a full regression for this and inspect the coverage yet?

Thank you @vogelpi for the review. I ran a full regression and all the transitions are now properly covered (see screenshot). I further added a second commit that updates the exclusion file with the list in #24692. @martin-velay you might want to double-check this.

Screenshot from 2024-12-18 15-43-29

@martin-velay
Copy link
Contributor

Hi @andrea-caforio, I would prefer to have the exclusions in a separate PR as there is no direct link between this RTL fix and the exclusions. Thanks in advance !

This commit removes some conditional FSM states transition that were
deemed undesirable in `prim_sha2_pad`, see lowRISC#23936. They made it
possible to cancel an ongoing padding operation, which is disallowed
by the HMAC module.

This change makes it impossible to transition into the `StFifoReceive`
state from either `StPad80`, `StPad00`, `StLenHi` or `StLenLo` by
asserting the `hash_go` signal.

Signed-off-by: Andrea Caforio <[email protected]>
@andrea-caforio andrea-caforio force-pushed the hmac-disallow-padding-cancellation branch from 8d79a9b to 54425d2 Compare December 18, 2024 15:07
@andrea-caforio
Copy link
Contributor Author

Hi @andrea-caforio, I would prefer to have the exclusions in a separate PR as there is no direct link between this RTL fix and the exclusions. Thanks in advance !

Here you go #25696.

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.

4 participants