lowRISC / lowRISC/opentitan

[rom_ctrl] ROM verification does not make use ECC bits

Open
#28,802 3 comments 0 reactions 0 assignees View on GitHub
IP:rom_ctrl
Dominant language
SystemVerilog
Stars
3.6k
Forks
1.1k
Avg merge
2d 22h
Merged PRs (30d)
141

Description

### Description

It is my understanding that rom_ctrl only uses the 32 data bits of the ROM when doing the verification via KMAC and it ignores the ECC bits: https://github.com/lowRISC/opentitan/blob/1f7db17392086db69b4f3f161e3d02f59f7dfb0d/hw/ip/rom_ctrl/rtl/rom_ctrl.sv#L379
However, when reading from the ROM via the TLUL interface, the ECC bits are preserved.

This means that if there is a wrong bit in the data part of the ROM, e.g., due to manufacturing defects, then the ROM cannot be verified because the ECC bits are not being used to correct the data.

This also means that the ECC part of the ROM are currently wasted area. They are only used when reading data via the TLUL interface but to get to the point the ROM verification needs to happen first, which will always fail if there is an error in the data bits.

**Open Questions:**
I did not find the point where the ECC bits in the TLUL response are being used to correct any data. Is this the case or does ibex simply raise an alert when there is a (correctable) error?

**Possible Solution:**
I propose to use ECC and correct any data errors before passing the data to KMAC.
I think ideally this is being done close to the ROM itself similar to how prim_ram_1p_adv does ECC data correction before anyone can consume it.

However, the digest part of the ROM is currently not ECC encoded (or scrambled) according to [this comment](https://github.com/lowRISC/opentitan/issues/16072#issuecomment-1306170540).
This means that the correction/decoding should only happen when not reading the digest, or we store ECC bits for the unscrambled digest in the ROM. I prefer the former in order not to break any existing flows. However we should discuss this and carefully consider our options.

**More Context:**
This issue has also been discussed in #16072 but mostly only from a security perspective.
I am arguing that we should make use of the ECC bits that we are storing anyway to increase reliability.

**Implementation Notes:**
When making this change we should probably also update `chip_sw_rom_ctrl_integrity_check_vseq` and corrupt non-digest data as well.

Contributor guide

Open the contributing guide

Research direction

Start with hw/ip/rom_ctrl/rtl/rom_ctrl.sv around the KMAC verification path linked in the issue, then compare its ECC handling with prim_ram_1p_adv. Read chip_sw_rom_ctrl_integrity_check_vseq and the discussion in #16072 to resolve how digest data differs from other ROM data. Done means the chosen ECC behavior is implemented and the sequence covers corruption of non-digest data.

Written by the indexing model from the issue text.

Assessment

Domain
embedded-iot, security
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.