[flash_ctrl] ICV failure on single bit flipped erased words with scrambling

[sasdf]

Description

In the current flash_phy_rd implementation, the data_erased signal is used to bypass the descrambling logic when an unprogrammed (erased) word is read.

opentitan/hw/top_earlgrey/ip_autogen/flash_ctrl/rtl/flash_phy_rd.sv

Lines 494 to 495 in e86869f

	// descramble is only required if the location is scramble enabled AND it is not erased.
	assign descram = rd_done & rd_attrs.descramble & ~data_erased;

A single bit flip within an erased word should be correctable by the reliability ECC. However, because data_erased is calculated using the raw, uncorrected data (data_i), a single flip causes data_erased to be de-asserted.

opentitan/hw/top_earlgrey/ip_autogen/flash_ctrl/rtl/flash_phy_rd.sv

Lines 431 to 433 in e86869f

	// When all bits are 1, the data has been erased
	// This check is only valid when read data returns.
	assign data_erased = rd_done & (data_i == {FullDataWidth{1'b1}});

(Note: data_i = content + ICV + ECC)

The hardware then incorrectly attempts to descramble the word. This results in corrupted data and a subsequent integrity check failure, effectively making the single-bit error uncorrectable.

CC: @vogelpi, @moidx, @johannheyszl

[rswarbrick]

I've just read through this carefully and I think I understand the description of the problem. Basically, we start with data that has been erased (so has value 'b1111...111) and a single bit flip turns turns it into e.g. 'b1111... 1110111...111. The result is that data_erased goes low and we "descramble" the bogus data, resulting in a right mess.

Since we already pass everything through u_dec before sticking it into the data fifo, I guess one option would be to apply the ECC decoder in the path defining data_erased. I think we could do that by defining data_erased = rd_done & &data_ecc_chk (possibly with ... & ~ecc_multi_err).

I think this would solve the "injected single bit error" problem, but I can see one obvious question: does the timing then work? We end up adding more combinatorial logic into the descram signal. That feeds into forward and the input to u_rd_storage. I wouldn't expect to need to worry about timing on the fifo, but forward seems to be a forwarding path that feeds into things like muxed_data. This then feeds into data_ctrl_o, so we would be adding an extra ECC decode to a combinatorial path from data_i to data_ctrl_o.

---

Breadcrumb trail: For anyone trying to trace the history of this file, a big rename operation in 2024 made things a bit more difficult. You can get the full log with e.g. git log -- '*flash_phy_rd.sv' and I think the last commit that touched the file before the rename was 8046c2896fa50aaf3a. As a result, you can get the history of the code in question with git log -p 8046c2896fa50aaf3a -- hw/ip/flash_ctrl/rtl/flash_phy_rd.sv. The code that checks when data is erased was added by Tim Chen in Sep 2020 with e96730f.

[vogelpi]

I don't remember exactly but I think it was a conscious decision to not use the ECC corrected word to detect whether the word has been erased (IIRC, ECC can be enabled/disabled per partition).

@gautschimi , could you please take a look at this? @rswarbrick 's breadcrumb trail might be very helpful for finding out why the design is as it is.

[gautschimi]

A bit of background:

First, we use prim_secded_hamming_76_68_enc to create the 8b ECC. The ECC of all-one is 0xFF (also all-one). hence, an erased word has a valid ECC. -> I think there is no good reason to disable reliability ECC decoding/correction to check if a word has been erased.

Second, the ICV is computed on unscrambled data with prim_secded_hamming_72_64_enc. This is the same architecture as the reliability ECC. The ICV of an all-one word will also be all-one, and hence valid.

In earlier commits of flash_ctrl a different secded was used where an all-one word did not have a all-one ECC. Hence, after erasing data and ECC would not match anymore. Therefore, ECC decoding was disabled when data_erased=1 was detected. This was changed in 4076187 and 4798f27.

possible solutions:

1. In flash_phy_rd we disable descrambling if rd_done & (data_i == '1). I think what @rswarbrick is proposing could be one solution for the described problem. data_erased = rd_done & (data_ecc_chk == '1). ECC would still be valid thanks to secded_hamming and can be used for decoding.

2. Alternatively, we could add a new attribute for the controller read command read_unscrambled that would disable the descrambling for the given command.

Both solutions require hardware changes. I personally don't like that we check for all-one in hardware. What if the actual scrambled value is indeed exactly all-one? Maybe it is guaranteed that the ICV and scrambled value cannot be all one at the same time. I would probably prefer solution 2)

Meanwhile, a software workaround would be to define a region for the erased section and disabled scrambling in software for this region. This is probably only possible if the region is not locked.

[sasdf]

Possible software workarounds include one of the following options:

1. Restricting read op to programmed locations only.
2. Explicitly writing all-ones to these words.
3. Disable scrambling for the ECC enabled regions.

Are there other workarounds worth exploring?

---

What if the actual scrambled value is indeed exactly all-one? Maybe it is guaranteed that the ICV and scrambled value cannot be all one at the same time.

It is possible to intentionally trigger this condition with crafted data if the per-device scrambling keystream is known, while the probability of it occurring accidentally is only 1/2^68. I agree that a dedicated indicator would still be a better choice for a secure processor (this indicator also needs to be ECC corrected for reliability).

[gautschimi]

Possible software workarounds include one of the following options:

1. Restricting read op to programmed locations only.

That would work

2. Explicitly writing all-ones to these words.

You can only write zeros. if you want to set something back to 1 you need to erase the full sector.

3. Disable scrambling for the ECC enabled regions.

I wouldn't disable it globally. But if you want to verify that something is indeed erased, I would suggest to define a new software region for the erased sector and disable scrambling for that region. (lower rules have priority). The rule can be disabled again after the check. This only works for data partitions, for info pages you could use the info_page_override mechanism: https://opentitan.org/book/hw/ip/flash_ctrl/doc/registers.html#hw_info_cfg_override

Are there other workarounds worth exploring?

If you just want to verify that something is fully erased, I would propose to do what I described above.

What if the actual scrambled value is indeed exactly all-one? Maybe it is guaranteed that the ICV and scrambled value cannot be all one at the same time.

It is possible to intentionally trigger this condition with crafted data if the per-device scrambling keystream is known, while the probability of it occurring accidentally is only 1/2^68. I agree that a dedicated indicator would still be a better choice for a secure processor (this indicator also needs to be ECC corrected for reliability).

ICV and data is linked, so the probability might actually be 0, it depends on the scrambling keys. You can descramble 0xFFFF_FFFF_FFFF_FFFF and check if the ICV of the result is all-one.

[sasdf]

ICV and data is linked, so the probability might actually be 0, it depends on the scrambling keys. You can descramble 0xFFFF_FFFF_FFFF_FFFF and check if the ICV of the result is all-one.

The scrambling uses XEX mode which depends on the transaction address, so the 0xF ICV with descrambled all-ones will occur once every 16 flash addresses on average.

[gautschimi]

ICV and data is linked, so the probability might actually be 0, it depends on the scrambling keys. You can descramble 0xFFFF_FFFF_FFFF_FFFF and check if the ICV of the result is all-one.

The scrambling uses XEX mode which depends on the transaction address, so the 0xF ICV with descrambled all-ones will occur once every 16 flash addresses on average.

True, I forgot about the address component. In that case it can happen. So an all-one word is actually legal and descrambling would be skipped. I guess this should be adjusted in hardware