[kmac] fatal and recoverable alerts seem to be mixed up

[m-temp]

While having a look at KMAC it seems like the fatal and recoverable alerts got mixed up in https://github.com/lowRISC/opentitan/pull/7439

In kmac.sv lines 1131-1163 it states:

assign alert_test = {
    reg2hw.alert_test.recov_operation_err.q
      & reg2hw.alert_test.recov_operation_err.qe, // [1]
    reg2hw.alert_test.fatal_fault_err.q
      & reg2hw.alert_test.fatal_fault_err.qe          // [0]
  };

  assign alerts = {
    alert_recov_operation, // Alerts[1]
    alert_fatal            // Alerts[0]
    };

  assign alert_recov_operation = shadowed_update_err;

  assign alert_fatal = shadowed_storage_err
                     | alert_intg_err
                     ;

  for (genvar i = 0; i < NumAlerts; i++) begin : gen_alert_tx
    prim_alert_sender #(
      .AsyncOn(AlertAsyncOn[i]),
      .IsFatal(i)
    ) u_prim_alert_sender (
      .clk_i,
      .rst_ni,
      .alert_test_i  ( alert_test[i] ),
      .alert_req_i   ( alerts[i]     ),
      .alert_ack_o   (               ),
      .alert_state_o (               ),
      .alert_rx_i    ( alert_rx_i[i] ),
      .alert_tx_o    ( alert_tx_o[i] )
    );
  end

IsFatal(i) is true for i=1, but the fatal alert is declared to be [0]. Do I miss here something related to vector and arrays? Anyways, if in doubt, this should be rewritten in a more intuitive way

[eunchan]

Thanks for finding this. I will fix it.

[m-temp]

I'm not a DV expert, but is there any way to detect such kinds of errors automatically?

[cindychip]

Hi Michael, Yes, actually it could be detected by shadow_reg tests, but the tests are gated by this discussion - https://github.com/lowRISC/opentitan/issues/8460 I can probably align it, then we can see the error that the recoverable alert keeps firing.

I actually check the integrity error, even though it is a recoverable alert, it still fires continuously like a fatal alert because the intg_err signal is asserted high on reset. Then the rest of the fatal errors are security related, it is not implemented yet and we should cover them in V2S stage.

Please let me know if you have any questions. Thanks, Cindy

On Thu, Feb 3, 2022 at 7:50 AM Michael Tempelmeier @.***> wrote:

I'm not a DV expert, but is there any way to detect such kinds of errors automatically?

— Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/10516#issuecomment-1029129240, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXPOOJLCQSNZHGFV4MGISLUZKP5HANCNFSM5NJUCZ2Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

[m-temp]

Hi Cindy, thx for the info.

I was just curious, because prim_count, prim_double_lfsr, prim_sparse_fsm also enforce an ASSERT_<PRIM_NAME>_ERROR_TRIGGER_ALERT but it looks like they didn't catch it neither. Even worse, when the alerts were changed in #10517 the corresponding assertions haven't been updated and there was probably no CI failure? #10562 fixed that as an side effect. Or do I miss something here?

You are right, this is security related, but it would be good to catch that. Perhaps those assertions could have an option for the kind of alert (fatal) and then double check that the corresponding alert-sender is configured as fatal?

Michael

[cindychip]

Thanks Michael,

1). About assertion firing with change https://github.com/lowRISC/opentitan/pull/10517, that is a good point! We did not see any assertion firing because we did not have stimulus to trigger or reach the assertion. If we refer to the assertion coverage report https://reports.opentitan.org/hw/ip/kmac_masked/dv/latest/cov_report/asserts.html, they are currently classified as uncovered. (pasted the screenshot below :) We should be able to see these coverage when V2S tests are created.

2). That is a good point to add assertions to check the alert is fatal! I will talk to the team about this idea.

Thanks again for pointing these out. Definitely good reminders and I always appreciate feedback from the designer :)

[image: image.png]

Cindy Chen

On Fri, Feb 4, 2022 at 2:15 AM Michael Tempelmeier @.***> wrote:

Hi Cindy, thx for the info.

I was just curious, because prim_count, prim_double_lfsr, prim_sparsefsm also enforce an ASSERT_ERROR_TRIGGER_ALERT but it looks like they didn't catch it neither. Even worse, when the alerts were changed in

10517 https://github.com/lowRISC/opentitan/pull/10517 the

corresponding assertions haven't been updated and there was probably no CI failure? #10562 https://github.com/lowRISC/opentitan/pull/10562 fixed that as an side effect. Or do I miss something here?

You are right, this is security related, but it would be good to catch that. Perhaps those assertions could have an option for the kind of alert (fatal) and then double check that the corresponding alert-sender is configured as fatal?

Michael

— Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/10516#issuecomment-1029833086, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXPOOMWQVDNYNM7XOFYC43UZORMNANCNFSM5NJUCZ2Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

[msfschaffner]

@eunchan has this bug been addressed? I think the alert connections are fixed by now, right?