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

Entropy_Src D2S Review AIs #10096

Closed
19 tasks done
mwbranstad opened this issue Jan 14, 2022 · 6 comments · Fixed by #10486
Closed
19 tasks done

Entropy_Src D2S Review AIs #10096

mwbranstad opened this issue Jan 14, 2022 · 6 comments · Fixed by #10486

Comments

@mwbranstad
Copy link
Contributor

mwbranstad commented Jan 14, 2022

Action Items:

@mwbranstad
Copy link
Contributor Author

@msfschaffner all the boxes are checked now. Should we close this issue, or are there some additional steps to do before that happens in the security review flow?

mwbranstad added a commit to mwbranstad/opentitan that referenced this issue Jan 31, 2022
Approval of this PR will close all related review issues and signify
that entropy_src, csrng, and edn blocks have completed D2S checkpoints.
Closes lowRISC#10095.
Closes lowRISC#10096.
Closes lowRISC#10262.

Signed-off-by: Mark Branstad <[email protected]>
@msfschaffner
Copy link
Contributor

msfschaffner commented Jan 31, 2022

@mwbranstad we can close it once the D2S prj.hjson has landed. You could even do this automatically by adding Fixes #10096 to the commit message of the D2S prj.hjson commit.

Edit: I just noticed that you already seem to be using closes XYZ in the PR description - so that is fine. We will close the issues once that PR goes in.

@msfschaffner
Copy link
Contributor

@mwbranstad, a few questions regarding the D2S opens:

  1. Should [entropy_src] RFC: Programming Model for FW_OV_ENTROPY_INSERT #10388 be addressed before we declare D2S?
  2. Can you add the link to the following issue? I don't seem to find that one [Martin] File issue for future improvements: split out fatal error conditions into indication CSRs
  3. In general it would be good to indicate where things have been addressed if possible, either providing the link above per item, or grouping the items as for instance done in: [otp_ctrl] D2S review opens #10420. There are still a couple of items above without link, and since many changes are squashed into large commits it is hard to trace what exactly has been addressed where.

This was referenced Jan 31, 2022
@mwbranstad
Copy link
Contributor Author

@mwbranstad, a few questions regarding the D2S opens:

  1. Should [entropy_src] RFC: Programming Model for FW_OV_ENTROPY_INSERT #10388 be addressed before we declare D2S?
  2. Can you add the link to the following issue? I don't seem to find that one [Martin] File issue for future improvements: split out fatal error conditions into indication CSRs
  3. In general it would be good to indicate where things have been addressed if possible, either providing the link above per item, or grouping the items as for instance done in: [otp_ctrl] D2S review opens #10420. There are still a couple of items above without link, and since many changes are squashed into large commits it is hard to trace what exactly has been addressed where.

A1: #10388 can be added to the list. It did not start out as a security topic, but morphed into one.
A2: It was determined that the ERROR_CODE status register does this function, so no issue was opened.
A3: Will review again, but thought every AI had either a link or some explanation.

@msfschaffner
Copy link
Contributor

msfschaffner commented Feb 2, 2022

Thanks @mwbranstad. It looks like the only pending item is #10520 - after that entropy_src appears to be good to go.

@msfschaffner
Copy link
Contributor

Ok #10520 has been merged, thanks to @tjaychen's help.
There is one more minor RTL fix that has to go in (#10621) and then we should be g2g here.

msfschaffner pushed a commit to mwbranstad/opentitan that referenced this issue Feb 4, 2022
Approval of this PR will close all related review issues and signify
that entropy_src, csrng, and edn blocks have completed D2S checkpoints.
Closes lowRISC#10095.
Closes lowRISC#10096.
Closes lowRISC#10262.

Signed-off-by: Mark Branstad <[email protected]>
msfschaffner pushed a commit to mwbranstad/opentitan that referenced this issue Feb 4, 2022
Approval of this PR will close all related review issues and signify
that entropy_src, csrng, and edn blocks have completed D2S checkpoints.
Closes lowRISC#10095.
Closes lowRISC#10096.
Closes lowRISC#10262.

Signed-off-by: Mark Branstad <[email protected]>
Signed-off-by: Michael Schaffner <[email protected]>
msfschaffner pushed a commit that referenced this issue Feb 7, 2022
Approval of this PR will close all related review issues and signify
that entropy_src, csrng, and edn blocks have completed D2S checkpoints.
Closes #10095.
Closes #10096.
Closes #10262.

Signed-off-by: Mark Branstad <[email protected]>
Signed-off-by: Michael Schaffner <[email protected]>
luismarques pushed a commit to luismarques/opentitan that referenced this issue Feb 9, 2022
Approval of this PR will close all related review issues and signify
that entropy_src, csrng, and edn blocks have completed D2S checkpoints.
Closes lowRISC#10095.
Closes lowRISC#10096.
Closes lowRISC#10262.

Signed-off-by: Mark Branstad <[email protected]>
Signed-off-by: Michael Schaffner <[email protected]>
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 a pull request may close this issue.

8 participants