[otbn/sca] Add new sca features for otbn_ecdsa_256 #16899

bilgiday · 2022-12-20T03:24:00Z

Enable OTBNsim simulation of p256_ecdsa_sca.s, and enable receiving of all secret input shares(k0, k1, d0, d1) from the host-side Python code.

*.s file:

Change file to enable OTBNsim simulation.
Set default values for k0, k1, msg, x, y, d0, d1.
This values can be modified to compare the OTBNsim results to FPGA results.

*.c file:

Remove all the LOG_INFO between the two triggers to make sure the measured ECDSA operation's timing will be same for all runs.
Enable receiving of k0, k1, d0, d1 from the host-side Python code.
Add a new simpleserial command ('k') to separate the start_ecdsa,'p', and set secret scalar 'k'.

Signed-off-by: Bilgiday Yuce bilgiday@google.com

bilgiday · 2022-12-20T03:27:01Z

This is to enable receiving of the shares of the secret inputs from the Python side.
The Python counterpart is here: ot-sca-pr109.

Tested on FPGA with Husky. It seems to be working.

vogelpi

Thanks @bilgiday , this looks mostly good.

sw/device/sca/ecc_serial.c

vogelpi · 2022-12-22T14:40:35Z

sw/device/sca/ecc_serial.c

@@ -202,15 +249,6 @@ static void p256_ecdsa_sign(const uint32_t *msg, const uint32_t *private_key_d,
 */
 static void ecc256_ecdsa(const uint8_t *ecc256_secret_k_bytes,


This function has a description in the comment above that also needs to be aligned.

In addition, both the ecc256_secret_k_bytes and secret_k_len arguments are no longer used. So they should probably be removed from the function declaration/header.

The first point here (aligning function documentation) is still open. Can you please address this, too?

Ah, good catch, thanks! I updated the function documentation too.

sw/otbn/crypto/p256_ecdsa_sca.s

jadephilipoom · 2023-01-17T15:07:35Z

sw/device/sca/ecc_serial.c

@@ -157,33 +203,28 @@ static void p256_ecdsa_sign(const uint32_t *msg, const uint32_t *private_key_d,
                            uint32_t *signature_r, uint32_t *signature_s,
                            const uint32_t *k) {
  uint32_t mode = 1;  // mode 1 => sign
-  LOG_INFO("Copy data");


I think you don't technically need to remove these LOG_INFO calls, since the trigger only actually gets set when otbn_execute() is called. You just need to remove the ones after otbn_execute().

Yep, you are right. Previously, I put LOG_INFO calls to solve an issue. Now, they are kind of useless. In the new version, I converted them into comments for code readability. Would it be okay for you?

In the past we noticed that such outputs in the hot paths had a significant effect on throughput. I think it's ok to leave them out unless there is a particular reason we want them.

Yep, I agree it's fine to remove them anyway.

jadephilipoom · 2023-01-17T15:07:47Z

sw/device/sca/ecc_serial.c

  SS_CHECK(otbn_dmem_write(kEcc256NumWords, msg, kOtbnVarMsg) == kOtbnErrorOk);
+  // Send two sahres of private_key_d to OTBN


nit:

Suggested change

// Send two sahres of private_key_d to OTBN

// Send two shares of private_key_d to OTBN

thanks! fixed it.

vogelpi

From my side this looks good, thanks for addressing the comments @bilgiday !

There is one minor thing open (update function description). I think it's in general very important to keep code comments up to date. Otherwise they are a bit useless.

alphan · 2023-01-17T19:49:19Z

sw/device/sca/ecc_serial.c

@@ -157,33 +203,28 @@ static void p256_ecdsa_sign(const uint32_t *msg, const uint32_t *private_key_d,
                            uint32_t *signature_r, uint32_t *signature_s,
                            const uint32_t *k) {
  uint32_t mode = 1;  // mode 1 => sign
-  LOG_INFO("Copy data");


In the past we noticed that such outputs in the hot paths had a significant effect on throughput. I think it's ok to leave them out unless there is a particular reason we want them.

alphan · 2023-01-17T19:57:15Z

sw/otbn/crypto/p256_ecdsa_sca.s

-.bss
+.data



@bilgiday @jadephilipoom Can you check the size of the binary? IIUC this change will put these constants in the OTBN binary which will increase the size of anything that it will be linked into.

Thanks Alphan, good point. Let me provide more context here:

I made that change to be able to run the p256_ecdsa_sca.s in the OTBNsim; I was getting compilation errors when I use .bss. I thought that change would be okay because we are using this binary only for side-channel analysis, and we are not linking it to any production code.

In this context, would it be okay to keep the .data as it is?

alphan · 2023-01-17T19:58:35Z

sw/otbn/crypto/p256_ecdsa_sca.s


 /* message digest */
 .globl msg
 .balign 32
 msg:
-  .zero 32
+  /*"Hello OTBN."*/
+  /* Word-order: The word with the lowest index is last */


I was surprised when I first read this but I think the comment (all occurences) needs to be fixed:

Suggested change

/* Word-order: The word with the lowest index is last */

/* Word-order: The word with the lowest index is first */

Thanks, the comment was misleading and redundant. I removed that comment. Instead, I just keep the value I intend to set. For example, /* msg = 0x06d71207...4456fd21 */.

Enable OTBNsim simulation of p256_ecdsa_sca.s, and enable receiving of all secret input shares(k0, k1, d0, d1) from the host-side Python code. *.s file: - Change file to enable OTBNsim simulation. - Set default values for k0, k1, msg, x, y, d0, d1. - This values can be modified to compare the OTBNsim results to FPGA results. *.c file: - Remove all the LOG_INFO between the two triggers to make sure the measured ECDSA operation's timing will be same for all runsi. - Enable receiving of k0, k1, d0, d1 from the host-side Python code. - Add a new simpleserial command ('k') to separate the start_ecdsa,'p', and set secret scalar 'k'. Signed-off-by: Bilgiday Yuce <bilgiday@google.com>

This was referenced Dec 20, 2022

[ecdsa-sca] Enable ECDSA-256 secret shares for capture lowRISC/ot-sca#109

Merged

[ecdsa] Extend setup to allow for random and fixed data lowRISC/ot-sca#106

Open

vogelpi reviewed Dec 22, 2022

View reviewed changes

bilgiday force-pushed the otbn-sca-latest branch from e97c680 to f73a66b Compare January 17, 2023 15:02

bilgiday marked this pull request as ready for review January 17, 2023 15:02

bilgiday requested a review from a team as a code owner January 17, 2023 15:02

bilgiday requested review from alphan, jadephilipoom, a team and vogelpi and removed request for a team January 17, 2023 15:02

jadephilipoom approved these changes Jan 17, 2023

View reviewed changes

vogelpi approved these changes Jan 17, 2023

View reviewed changes

bilgiday force-pushed the otbn-sca-latest branch from f73a66b to 58b32ca Compare January 17, 2023 19:42

alphan reviewed Jan 17, 2023

View reviewed changes

bilgiday force-pushed the otbn-sca-latest branch from 58b32ca to f82ea0c Compare January 18, 2023 13:27

bilgiday requested a review from alphan January 19, 2023 09:44

jadephilipoom merged commit dd41ae9 into lowRISC:master Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[otbn/sca] Add new sca features for otbn_ecdsa_256 #16899

[otbn/sca] Add new sca features for otbn_ecdsa_256 #16899

bilgiday commented Dec 20, 2022

bilgiday commented Dec 20, 2022

vogelpi left a comment

vogelpi Dec 22, 2022

vogelpi Dec 22, 2022

vogelpi Jan 17, 2023

bilgiday Jan 17, 2023

jadephilipoom Jan 17, 2023

bilgiday Jan 17, 2023

alphan Jan 17, 2023

jadephilipoom Jan 18, 2023

jadephilipoom Jan 17, 2023

bilgiday Jan 17, 2023

vogelpi left a comment

alphan Jan 17, 2023

alphan Jan 17, 2023

bilgiday Jan 18, 2023

alphan Jan 17, 2023

bilgiday Jan 18, 2023

		@@ -202,15 +249,6 @@ static void p256_ecdsa_sign(const uint32_t msg, const uint32_t private_key_d,
		*/
		static void ecc256_ecdsa(const uint8_t *ecc256_secret_k_bytes,

		SS_CHECK(otbn_dmem_write(kEcc256NumWords, msg, kOtbnVarMsg) == kOtbnErrorOk);
		// Send two sahres of private_key_d to OTBN

	// Send two sahres of private_key_d to OTBN
	// Send two shares of private_key_d to OTBN

	/* Word-order: The word with the lowest index is last */
	/* Word-order: The word with the lowest index is first */

		.bss
		.data

[otbn/sca] Add new sca features for otbn_ecdsa_256 #16899

[otbn/sca] Add new sca features for otbn_ecdsa_256 #16899

Conversation

bilgiday commented Dec 20, 2022

bilgiday commented Dec 20, 2022

vogelpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogelpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment