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

TLUL Error Response and devmode #10311

Closed
weicaiyang opened this issue Jan 24, 2022 · 8 comments · Fixed by #20861
Closed

TLUL Error Response and devmode #10311

weicaiyang opened this issue Jan 24, 2022 · 8 comments · Fixed by #20861
Assignees
Labels
Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones Priority:P1 Priority: high

Comments

@weicaiyang
Copy link
Contributor

  1. when there is an error on the read, we currently respond all 1s for CSR read and all 0s for mem read (all 0s is an illegal instruction in RISC-V [sram_ctrl] D2S review opens #10195). I'm fine with this. LMK if this is a concern.

  2. We have defined a devmode, but looks like this port hasn't been exposed to the IP top and it's tied to 1 in all IPs. Is below spec still valid or should we remove it?

for security modules (Comportability definition forthcoming), this is under the control of a register module input signal devmode_i. This signal indicates whether the whole SOC device is in development or production mode. For security peripherals in production mode, it is desired to not send an error response, so write misses silently fail, and read misses silently fail, but return either random data (TBD) or all 1s for response data (i.e. 0xFFFFFFFF for a 32b register). For non-security peripherals, or when in development mode (devmode_i == 1) these writes and reads to undefined addresses will return with TL-UL error response.

@tjaychen
Copy link

@moidx
@felixmiller

let's bring this up in the next security meeting. This is very related to an issue that we ran out of time to discuss last week.
Which is basically, what level of debug detail do we give the software?

This is more or less a philosophical decision of the entire project. Early on we decided to show very little, but that trend is somewhat shifting in certain areas, and we need to make a collective decision on the project.

My personal opinion is that this particular project we don't need to hide from software (we have a few mechanisms that can prevent software abuse, but we don't generally work against it). Other projects I can very reasonably see why we want to hide more.

@moidx moidx added the Hotlist:Security Security Opinion Needed label Jan 25, 2022
@tjaychen
Copy link

tjaychen commented Feb 4, 2022

After discussions today, it was decided that for OpenTitan's "current usecase", there is no need to hide the tlul error from software.
Further, exposing these errors may be beneficial for in the field debugging.

For future usecases, this question should be re-examined as it may be a very reasonable choice to lock down the error response and indication.

@tjaychen tjaychen closed this as completed Feb 4, 2022
@weicaiyang
Copy link
Contributor Author

Thanks @tjaychen for the update.

@vogelpi
Copy link
Contributor

vogelpi commented Apr 24, 2023

Removing the "Hotlist:Security" label as this issue/label is very old and the issue has been marked as "FutureRelease"

@vogelpi vogelpi removed the Hotlist:Security Security Opinion Needed label Apr 24, 2023
@msfschaffner msfschaffner added the Hotlist:Security Security Opinion Needed label Oct 7, 2023
@msfschaffner
Copy link
Contributor

We should probably double check whether our assessment still applies for PROD.

@msfschaffner msfschaffner added the Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones label Oct 7, 2023
@vogelpi
Copy link
Contributor

vogelpi commented Oct 24, 2023

For clarification, we always return all 1s (peripherals) or all 0s (memory) in the response data when accessing unmapped addresses. The only thing devmode_i controls is whether the response error signal is high or not.

After discussing with @msfschaffner and @johannheyszl , we think that the previous assumptions still hold for our current designs and use cases: 1) exposing the error condition eases debugging in the field and software development, 2) from a security point of view there seems to be little use in hiding the error condition, 3) what is relevant for security is to blank the response data which is happening independent of devmode_i. We also think that we shouldn't change this at this point given that this affects many blocks (perhiperals + memory ports, Xbars etc.)

Thus we're going to suggest to leave this as is for the time being in the next Security working group meeting. Maybe we should think about updating the documentation to reflect our thinking there as well.

@vogelpi
Copy link
Contributor

vogelpi commented Oct 27, 2023

We have discussed this in the Security Working Group meeting on Oct 26, 2023 and concluded the following (on top of the points mentioned previously):

  • The option for hiding the error response was initially proposed to make it more difficult for adversaries to divert TL-UL traffic through FI (e.g. to bypass read protection handled by PMP close to the processor). This was before we implemented blanking of the response data and before we added full integrity on the address and data channels.
  • With response data blanking and full integrity on address and data channels in place it was found that hiding the error response would add very little to no extra security but complicate debugging and software development.
  • In addition, the devmode_i input which is currently statically driven creates challenges in DV (coverage) and the documentation currently suggests that leaving devmode_i on would pose a security risk.

Therefore, we have decided to not change the behavior of the design (error response is signaled together with the blanked response data) and to remove the devmode_i input from all peripherals and memories. This involves a minor RTL change to most hardware blocks but the behavior is left unchanged.

I am thus removing the hotlist label. My understandig is that we even want this for Earlgrey Prod, so we could also remove the future releases label. Is this correct @msfschaffner ?

@vogelpi vogelpi removed the Hotlist:Security Security Opinion Needed label Oct 27, 2023
@msfschaffner
Copy link
Contributor

msfschaffner commented Oct 27, 2023

Thanks for the summary, @vogelpi!

Yes, that is correct! It is also tagged as a PROD candidate.

@msfschaffner msfschaffner removed the Type:FutureRelease Not relevant to currently planned releases/milestones label Oct 27, 2023
@msfschaffner msfschaffner modified the milestones: Backlog, Earlgrey-PROD.M2 Nov 7, 2023
@msfschaffner msfschaffner added the Priority:P1 Priority: high label Nov 7, 2023
@msfschaffner msfschaffner self-assigned this Dec 21, 2023
msfschaffner added a commit to msfschaffner/opentitan that referenced this issue Jan 17, 2024
As discussed on lowRISC#10311 we are removing the devmode input
signal from the codebase, since it has been decided that this
will not be used.

Signed-off-by: Michael Schaffner <[email protected]>
@msfschaffner msfschaffner linked a pull request Jan 17, 2024 that will close this issue
msfschaffner added a commit to msfschaffner/opentitan that referenced this issue Jan 17, 2024
As discussed on lowRISC#10311 we are removing the devmode input
signal from the codebase, since it has been decided that this
will not be used.

Signed-off-by: Michael Schaffner <[email protected]>
msfschaffner added a commit to msfschaffner/opentitan that referenced this issue Jan 17, 2024
As discussed on lowRISC#10311 we are removing the devmode input
signal from the codebase, since it has been decided that this
will not be used.

Signed-off-by: Michael Schaffner <[email protected]>
vogelpi pushed a commit that referenced this issue Jan 18, 2024
As discussed on #10311 we are removing the devmode input
signal from the codebase, since it has been decided that this
will not be used.

Signed-off-by: Michael Schaffner <[email protected]>
This was referenced Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones Priority:P1 Priority: high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants