search

lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.44k stars 730 forks source link

[tlul] tlul_sram_byte data integrity check #8815

Open tjaychen opened 2 years ago

tjaychen commented 2 years ago

When a byte write is issued to memory, tlul_sram_byte is responsible for doing a read first to generate the new word-wise integrity.

However, the read data integrity is not checked as part of this generation. This potentially creates a security risk.

estimate 4

tjaychen commented 2 years ago

@weicaiyang

weicaiyang commented 2 years ago

Let's review this in D2S meeting

msfschaffner commented 2 years ago

@tjaychen, should we just check ECC on all SRAM reads in this case (like we do on incoming TL-UL transactions)? I.e., if we are adding this logic anyway, we might as let all read transactions benefir from it...

tjaychen commented 2 years ago

o sorry, this is due to lack of a tagging scheme. This is meant to be "future" improvement. I'm not too concerned about weaknesses in the byte case because we already KNOW it is weaker than a full word write / read.

msfschaffner commented 2 years ago

Ah got it, thanks for the clarification!

FYI @moidx @cfrantz

moidx commented 2 years ago

Tagging with software label as this needs to be added to the software implementation guidelines.

andreaskurth commented 1 year ago

Triaged for tlul. Are we sure we don't want to check the integrity of data read in tlul_sram_byte by adding a tlul_data_integ_dec and feeding its data_err_o into error_o? (This to me does not seem to be a complex change, though we might have to insert some flops to break overly long paths coming out of SRAMs.) Tagging @vogelpi for a security opinion

vogelpi commented 1 year ago

Thanks for tagging me @andreaskurth . After having studied the RTL I agree that the change would probably be rather simple and it would be a nice to have for M2.5. However, it's not critical. Thus, I label this as P3. If we don't have time to do it or things turn out more difficult, we can instead include information on this in the software guidelines.

msfschaffner commented 1 year ago

Given that byte-writes are already considered less secure due to the ECC recomputation, I would lump this into software guidance for now and forego the RTL change.

msfschaffner commented 1 year ago

@moidx since this is documentation-only, I'll move this to the M2.5 Backlog.

msfschaffner commented 4 months ago

@vogelpi @johannheyszl are we ok with leaving this in the documentation-only category for Earlgrey-PROD? If so, does it make sense to capture that in the SW guidelines?

johannheyszl commented 4 months ago

Both ways - adding to guidelines, or a modification - sgtm. If the fix is small and we can avoid one more item on the guidelines, that is a benefit. @vogelpi ?

msfschaffner commented 4 months ago

I would lean towards no change at this point, since this also introduces again more DV effort.

vogelpi commented 4 months ago

Ideally, we would have changed the design here but I agree to leave this as is for Earlgrey-PROD. There are other, higher priority things to be taken care of first.

vogelpi commented 3 months ago

This was discussed in the Sec WG meeting on Mar 14, 2024 . It was concluded to leave this as is.

There are also other reasons for not doing byte writes and we have already SW guidance for those. For example, byte-write operations are not favorable from an SCA viewpoint. In both cases, SW should first perform a word read and then insert the byte inside the ALU and write back the full word.