[otbn,dv] Fix CTRL_REDUN tests targeting bignum MAC
Nobody has claimed this yet.
- Dominant language
- SystemVerilog
- Stars
- 3.6k
- Forks
- 1.1k
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 141
Description
This issue tracks that the control redundancy checks in `otbn_ctrl_redun_vseq` for the BN MAC are currently missing. This test should ensure that, e.g., a manipulation of a blanker control signal is detected and results in an error. The test also covers other important signals which have a redundant (predecoded) signal.
All components / signals to be checked should have the `SEC_CM: DATA_REG_SW.SCA` tag. First off, in the current OTBN with SIMD design the following blankers miss this tag (it should be added):
- `u_add_mod_blanker`
- `u_acc_merger_blanker`
- `u_add_res_blanker`
**Problem**
In https://github.com/lowRISC/opentitan/pull/29466 the test for `zero_acc` is removed because the old implementation is wrong and the `zero_acc` signal does not exist anymore in the SIMD design. Here is an explanation why this test was wrong:
In the non-SIMD OTBN (898132fecd0a6c27ca17ffadc9a25ef10b17b6b0) the `operation_i.zero_acc` signal was used to:
- Generate the `acc_used` signal which controls the integrity error propagation.
- An attack on `zero_acc` could suppress the ACC integrity error. This is not what this test is covering.
- Generate the `expected_acc_rd_en` signal.
- This signal is used to check the actually used `predec_i.acc_rd_en` control signal for the blanker `u_acc_blanker`.
The test for `zero_acc` should check that an attack on the control signal for the `u_acc_blanker` is detected. This is done the same way for the `mac_en_i` signal.
However, the test for `zero_acc` is not meaningful / wrong because the test focuses on the decoded signal instead of the predecoded one which actually controls the blanker. A valid test would target the predecoded signal and then wants to see that the mismatch is detected. There is no reason why one would attack the decoded signal.
**Todo**
We must extend the test such that it includes all blanker control signals and targets the predecoded signals. In addtion, the test should also include decoded `mac_en_i` signal because it factors into some important signals like `operation_valid_o`. Note that the predecoded counterpart for the `mac_en_i` signal is `predec_i.op_en` which controls the blankers `u_operand_a_blanker` and `u_operand_b_blanker`.
Prior to the actual test implementation we should resolve https://github.com/lowRISC/opentitan/issues/29496 which cleans up the use of `mac_en_i`.
When fixing this, we should also change the `good_op` check in `otbn_ctrl_redun_vseq.sv:122` so it includes the new SIMD instructions.
```
- if (good_op <= otbn_pkg::AluOpBignumNot)
+ if (good_op <= otbn_pkg::AluOpBignumUnpk)
```
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
First resolve issue #29496, then inspect otbn_ctrl_redun_vseq.sv, especially the good_op check at line 122. Review the listed blankers and SEC_CM: DATA_REG_SW.SCA tags, and extend the control-redundancy coverage to the predecoded blanker signals and decoded mac_en_i. Done means all relevant signals are targeted and manipulations produce the expected error, with SIMD instructions included in good_op.
Written by the indexing model from the issue text.
Assessment
- Domain
- embedded-iot, security, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100