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

[prim_fifo] Security Enhancement #10069

Closed
msfschaffner opened this issue Jan 13, 2022 · 7 comments
Closed

[prim_fifo] Security Enhancement #10069

msfschaffner opened this issue Jan 13, 2022 · 7 comments
Labels
Component:RTL Component:Security Priority:P2 Priority: medium Type:Enhancement Feature requests, enhancements
Milestone

Comments

@msfschaffner
Copy link
Contributor

For certain security IP like the entropy complex, it would be nice if the FIFO primitive could be instantiated in a "secure" mode where the read / write pointers use a duplicated counter (prim_count). This however also implies that the FIFO primitives now gain an alert output. Considering the potential design impact at this stage, I would say we mark this as a future enhancement for now - but I am open to make it a mandatory change if we think it is critical.

@cdgori
Copy link

cdgori commented Jan 13, 2022

I haven't seen a case (yet) where it is must-have, so your proposal of future enhancement is ok for me (for now).

We'll see as we go through the various D2S reviews how many other blocks use fifo primitives.

@cdgori
Copy link

cdgori commented Jan 19, 2022

Further comment from offline discussion between @mwbranstad / me - in theory we could use a prim_fifo with mubi controls and status (i.e. a mubi4 for full, wvalid_i, wready_o, rvalid_o, rvalid_i ) then using "mubi gates" inside the prim.

This sort of makes sense for applications where the user of the fifo has all their controls already in mubi form (admittedly there are not many such cases, so this also could be future enhancement).

@tjaychen
Copy link

so this option was actually added to all the synchronous fifos.
Now there is a parameter option to "harden" the counters underneath.

I think at this point, it probably doesn't make sense to apply them to blocks like entropy. The blocks we do want them to apply them to are basically ones that will directly affect cpu execution, so things like flash/sram.

@msfschaffner
Copy link
Contributor Author

Are there FIFO instances in the Xbar that we still need to wire up?
I believe you already connected FIFOs inside flash_ctrl, right?

@cdgori
Copy link

cdgori commented Jun 29, 2022

so this option was actually added to all the synchronous fifos. Now there is a parameter option to "harden" the counters underneath.

I think at this point, it probably doesn't make sense to apply them to blocks like entropy. The blocks we do want them to apply them to are basically ones that will directly affect cpu execution, so things like flash/sram.

I think that is ok because we have the repetition checks between the various parts of the entropy complex and from the EDN to the endpoints. In general we were worried about attacks on the fifos inside entropy where the attacker could jam the read ptr to a value (or something similar by manipulating write ptr and filling the fifo with constant data).

If we did targeted swaps on some of the fifos in entropy, how hard would it be flip this parameter? (i.e. do we need to add more CSR bits and/or alert ORs for security violations?)

@tjaychen
Copy link

since the ports on the fifo are already there, the main change to "flip" this feature on is as you say, connect the error conditions to alerts / status.

to @msfschaffner point, the FIFO's in the tlul are not protected at the moment. We probably need to think a bit about how we would want them protected, because there's no IP to receive the alerts at the moment. Not sure if it would be better to just create permanent in-band errors.

@msfschaffner
Copy link
Contributor Author

yeah a simple way would be to feed errors within the fabric back in-band (e.g. in the error field / or by corrupting the ECC). another way would be to send these errors over to an existing comportable IP, such as the rv_ibex wrapper, and wire it up to an alert + CSR there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL Component:Security Priority:P2 Priority: medium Type:Enhancement Feature requests, enhancements
Projects
None yet
Development

No branches or pull requests

8 participants