lowRISC / ibex

Ibex is a small 32 bit RISC-V CPU core, previously known as zero-riscy.
https://www.lowrisc.org
Apache License 2.0
1.34k stars 523 forks source link

regfile_latch: oh_raddr_a_err signal generated based on raddr_b_int #2215

Open glaserf opened 2 days ago

glaserf commented 2 days ago

Observed Behavior

The onehot read address error signal for read port a in ibex_register_file_latch.sv is generated based on the read address for read port b (raddr_b_int).

https://github.com/lowRISC/ibex/blob/53888bcdf4ca3c07e5e715fb6386cb4cc643a61b/rtl/ibex_register_file_latch.sv#L110-L125

Expected Behavior

The onehot read address error signal for read port a should be generated based on the read address for read port a, like in ibex_register_file_ff.sv and ibex_register_file_fpga.sv.

Version of the Ibex source code

53888bc (part of master)

rswarbrick commented 2 days ago

Thanks for this report, and it looks like there was a typo in 35bbdb7be. @nasahlpa: Have I understood correctly and do we just need to change a character?

GregAC commented 2 days ago

Well spotted @glaserf!

Looks like this only effects the latch implementation of the register file, which is why I think we didn't find it previously (as we simulate with the FF based register file). We should consider refactoring here so the security hardening is factored out into some common module that works equally against the FF/Latch/FPGA implementation options.

glaserf commented 2 days ago

Looks like this only effects the latch implementation of the register file, which is why I think we didn't find it previously (as we simulate with the FF based register file).

This is what I supposed - I stumbled upon it as this file caused a conflict during merging to an internal development branch were we added some test structures for the non-secure Ibex variant.

Refactoring to avoid code duplification (well, tripflification, currently) and ease verification coverage sure sounds very reasonable to me 👍🏼

nasahlpa commented 2 days ago

Nicely spotted @glaserf! Indeed, this is a typo introduced with the recent changes in the RF hardening.

Fixed in #2216.