Skip to content

Conversation

@h-filali
Copy link
Contributor

@h-filali h-filali commented Oct 27, 2025

The trigger_fault_if_fg0 functions had a bug where we never trigger a fault. This new implementation should now correctly trigger the fault.

For the case where no fault is triggered, we load address 0 into w31. For the error case we try to load address 0 into w39 (which doesn't exist), which triggers a ILLEGAL_INSN error.

This PR is based on #28592

Comment on lines 107 to 100
/* If we get here, the flag must have been 1. Restore w31 to zero and return.
/* If we get here, the flag must have been 0. Restore w31 to zero and return.
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should be reverted: This version of the function creates the error if the flag is 0.

* the flag's value is likely clearly visible to an attacker through timing.
*
* @param[in] w31: all-zero
* @param[in] FG0.Z: boolean indicating fault condition
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment needs to be aligned with the actual implementation and the comments below. The function name suggests it creates an error if the flag is NOT 0, while the comment here says the opposite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry this is not addressed. Did you forget to push maybe? Can you please re-read the whole comment starting at line 19 and going to line 34? The mismatch is at lines 20, 22 and 24. Thanks.

@h-filali h-filali force-pushed the cryptolib-ecc-fix-flg-zero-err branch from 80b2bfc to 71f3306 Compare October 27, 2025 14:49
@h-filali
Copy link
Contributor Author

Thanks @vogelpi for the review, I addressed all your comments. PTAL

*
* @param[in] w31: all-zero
* @param[in] FG0.Z: boolean indicating fault condition
* @param[in] FG0.Z: boolean indicating fault condition when 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Also for this file, can you please check the function names and the header comments please? In particular:

  • Line 36 says this function triggers a fault if fault if FG0.Z is 1
    The function is then named trigger_fault_if_fg0_z (does the final z stand for the Z of the flag or for zero?)
    Line 60 is aligned with that (cause error if FG0.Z is 1)
  • Line 71 says the second function triggers a fault if FG0.Z is 0
    The function is named trigger_fault_if_fg0_not_z - I think this name is counterintuitive comparing it with the terminology / function name above
    Line 96 then again says the error is triggered if FG0.Z is 0.

Maybe the counterintuitive function name is what confused me also with the other comment still open. Can we please align things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If fg0_z means fg0 is zero, then the naming is correct. However it can clearly be interpreted differently. I also thought the function names should be changed but I didn't want to make this change about changing the namings. I'll change the naming in my next amendment.

@h-filali h-filali force-pushed the cryptolib-ecc-fix-flg-zero-err branch 4 times, most recently from 3aade91 to f2ad6f4 Compare October 30, 2025 08:31
@h-filali h-filali requested a review from a team as a code owner October 30, 2025 08:31
@h-filali h-filali requested review from alees24 and removed request for a team October 30, 2025 08:31
@h-filali h-filali force-pushed the cryptolib-ecc-fix-flg-zero-err branch from f2ad6f4 to 7fc93f8 Compare October 30, 2025 10:24
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.

Thanks @h-filali , this looks great now!

@h-filali h-filali added the CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 label Oct 30, 2025
@vogelpi
Copy link
Contributor

vogelpi commented Oct 30, 2025

FYI: #28592 has now been added to the merge queue. Once it's merged, this PR here needs to be rebased.

@h-filali h-filali force-pushed the cryptolib-ecc-fix-flg-zero-err branch from 7fc93f8 to c20217c Compare October 30, 2025 17:19
@h-filali
Copy link
Contributor Author

Thanks @vogelpi I rebased the PR so once CI passes this can be merged.

The trigger_fault_if_fg0 functions had a bug where we never
trigger a fault. This new implementation should now correctly
trigger the fault.

For the case where no fault is triggered, we load address 0 into
w31. For the error case we try to load address 0 into
w39 (which doesn't exist), which triggers a ILLEGAL_INSN error.

Signed-off-by: Hakim Filali <[email protected]>
@vogelpi vogelpi force-pushed the cryptolib-ecc-fix-flg-zero-err branch from c20217c to 7fc6955 Compare October 30, 2025 20:16
@vogelpi vogelpi added this pull request to the merge queue Oct 31, 2025
Merged via the queue into lowRISC:master with commit b776b4f Oct 31, 2025
47 checks passed
@lowrisc-ci
Copy link

lowrisc-ci bot commented Oct 31, 2025

Backport failed for earlgrey_1.0.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-28574-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-28574-to-earlgrey_1.0.0
git switch --create backport-28574-to-earlgrey_1.0.0
git cherry-pick -x 7fc6955a6e6056975fecfa7e0016f6960bf520f7

@lowrisc-ci lowrisc-ci bot added the Manually CherryPick This PR should be manually cherry picked. label Oct 31, 2025
@lowrisc-ci
Copy link

lowrisc-ci bot commented Oct 31, 2025

Backport failed for earlgrey_1.0.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-28574-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-28574-to-earlgrey_1.0.0
git switch --create backport-28574-to-earlgrey_1.0.0
git cherry-pick -x 7fc6955a6e6056975fecfa7e0016f6960bf520f7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 Manually CherryPick This PR should be manually cherry picked.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants