lowRISC / lowRISC/opentitan

[otbn,dv] Fix CTRL_REDUN tests targeting bignum MAC

Open
#29,495 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

IP:otbn
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.