opentitan
opentitan copied to clipboard
lowRISC
[tlul] tlul_sram_byte data integrity check
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, 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...
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.
Tagging with software label as this needs to be added to the software implementation guidelines.
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
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.
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.
@moidx since this is documentation-only, I'll move this to the M2.5 Backlog.
@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?
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 ?
I would lean towards no change at this point, since this also introduces again more DV effort.
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.
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.