Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sw/fi] Add firmware code for OTP data read FI penetration test #23156

Open
wants to merge 1 commit into
base: earlgrey_es_sival
Choose a base branch
from

Conversation

wettermo
Copy link
Contributor

This adds firmware code for FI pen-testing the OTP module.

The test sequence can be summarized in the following way:

  1. Read content from a selected OTP partition for comparison values
  2. Perform fault injection on nop instructions
  3. Read content from the same OTP partition again
  4. Compare output with previous output and check if values differ

The corresponding PR in lowRISC/ot-sca is lowRISC/ot-sca#367

@wettermo wettermo requested a review from a team as a code owner May 16, 2024 13:07
@wettermo wettermo requested review from moidx and nasahlpa and removed request for a team and moidx May 16, 2024 13:07
@wettermo wettermo force-pushed the earlgrey_es_sival_fi_otp_pr branch from b4ad314 to 95fe35d Compare May 16, 2024 13:15
Copy link
Member

@nasahlpa nasahlpa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM apart from some smaller remarks.

TRY(otp_vendor_test_dump(otp_read32_result_vendor_test_fi));

// Get registered alerts from alert handler.
reg_alerts = sca_get_triggered_alerts();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a FYI - it might be useful to also register the alerts triggered inside Ibex. You could do something similar to here:

// Read ERR_STATUS register.
dif_rv_core_ibex_error_status_t codes;
TRY(dif_rv_core_ibex_get_error_status(&rv_core_ibex, &codes));

Also applies to the test functions below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, could be useful. I'll add it.

Comment on lines 134 to 136
uj_output.alerts[0] = reg_alerts.alerts[0];
uj_output.alerts[1] = reg_alerts.alerts[1];
uj_output.alerts[2] = reg_alerts.alerts[2];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uj_output.alerts[0] = reg_alerts.alerts[0];
uj_output.alerts[1] = reg_alerts.alerts[1];
uj_output.alerts[2] = reg_alerts.alerts[2];
memcpy(uj_output.alerts, reg_alerts.alerts, sizeof(reg_alerts.alerts));

Would also work.

Also applies to the test functions below.

// Send result & status codes to host.
otp_fi_hwcfg_partition_t uj_output;
for (uint32_t i = 0; i < OTP_CTRL_PARAM_HW_CFG_SIZE / 4; i++) {
uj_output.hw_cfg_comp[i] = otp_read32_result_hw_cfg_comp[i];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you are not doing the comparison on the chip instead of sending back all data?

In terms of performance, it could be faster to do the comparison on the chip to avoid the communication overhead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a discussion with Oscar about this, and he said that regarding communication overhead this shouldn't really be relevant, because, compared to SCA, it's only possible to do a couple of FI runs per second, so that the com overhead doesn't have that much impact. Also, it could be nice to have the whole output available on the host to have a look at it in case of a successful FI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, thanks!

Comment on lines +239 to +263
// FI code target.
sca_set_trigger_high();

// Point for FI
NOP1000;
NOP1000;
NOP1000;
NOP1000;

sca_set_trigger_low();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check with the oscilloscope whether this trigger window stays constant when executing this function multiple times? If not, it might be worth to set the reseeding interval to max in the init, .e.g.:

TRY(sca_configure_entropy_source_max_reseed_interval());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by trigger window staying constant? And how is this related to the entropy source?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When doing measurements with the oscilloscope, I've noticed that sometimes the trigger windows (i.e., the time between sca_set_trigger_high() and sca_set_trigger_low()) is not constant. The root cause for this was that there were several entropy complex reseed that delayed the execution.

UJSON_SERDE_STRUCT(OtpFiVendortestPartition, otp_fi_vendortest_partition_t, OTPFI_VENDORTEST_PARTITION);

#define OTPFI_OWNERSWCFG_PARTITION(field, string) \
field(owner_sw_cfg_comp, uint32_t, 200) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these sizes coming from (e.g. 200)? Would it make sense to use variables here to make sure that we do not accidentally try sending too many / less data over UART back to the host?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are just the amount of 32-bit words each partition has (which afaik are fixed).
I can add some #define macros at the top of the file and replace the hardcoded numbers with them, so that its better comprehensible what these sizes represent.
For example:

// Partition sizes in 32-bit words
#define VENDORTEST_SIZE32 16
#define OWNERSWCFG_SIZE32 200
#define HWCFG_SIZE32 20
#define LIFECYCLE_SIZE32 22

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be good, thanks!

@wettermo wettermo force-pushed the earlgrey_es_sival_fi_otp_pr branch from 95fe35d to bf8be96 Compare May 31, 2024 08:04
This adds firmware code for FI pen-testing the OTP module.
The test sequence can be summarized in the following way:
  1. Read content from a selected OTP partition for comparison values
  2. Perform fault injection on nop instructions
  3. Read content from the same OTP partition again
  4. Compare output with previous output and check if values differ

Signed-off-by: Moritz Wettermann <[email protected]>
@wettermo wettermo force-pushed the earlgrey_es_sival_fi_otp_pr branch from bf8be96 to 039e7b1 Compare May 31, 2024 08:10
@wettermo
Copy link
Contributor Author

Thanks Pascal for the review, I added everything. I also updated the PR in ot-sca (lowRISC/ot-sca#367) accordingly.

@nasahlpa nasahlpa requested a review from vogelpi May 31, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants