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

Added dedicated LSU operand registers #988

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

Conversation

Seek64
Copy link

@Seek64 Seek64 commented Nov 5, 2024

Based on a discussion with @Silabs-ArjanB

This PR introduces dedicated registers for LSU operands, which prevents the operands of non-LSU instructions from leaking to the data memory interface.
In the current version, internal data is always encoded as an address and forwarded, even if data_req_o is low.
A connected device that does not conform to the OBI protocol (because it's buggy or malicious) could use this covert channel to undermine security features such as data_ind_timing.

I have implemented the identical fix in my setup for cv32e40s to ensure that non-LSU operands do not leak to the data memory interface.
There may still be invalid memory requests and thus stale data, but only for LSU instructions.
Also, I have not yet considered compressed instructions.

I have not run any functional verification with this fix implemented.

Signed-off-by: Lucas Deutschmann <[email protected]>
@Silabs-ArjanB
Copy link
Contributor

Hi @Seek64 Thank you for your pull request. At first glance it looks at least functionally correct (but we will do a formal run to prove that). Also at first glance I think it must be possible to sometimes prevent updating the ALU operands when load/store instructions are being execute (to save power). That additional power savings is of course unrelated to the issue that you were trying to solve, so maybe we can just to a separate pull request for that later. It will take some time before we can study this further and do the merge.

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.

2 participants