Skip to content

[sw,cryptolib] Fix trigger_fault_if_fg0 functions #28574

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 31, 2025
Merged

[sw,cryptolib] Fix trigger_fault_if_fg0 functions #28574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions sw/device/lib/crypto/impl/ecc/p256.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,12 @@ enum {
/*
* The expected instruction counts for constant time functions.
*/
kModeKeygenInsCnt = 574237,
kModeKeygenSideloadInsCnt = 574122,
kModeEcdhInsCnt = 581920,
kModeEcdhSideloadInsCnt = 581980,
kModeEcdsaSignInsCnt = 607411,
kModeEcdsaSignSideloadInsCnt = 607471,
kModeKeygenInsCnt = 573915,
kModeKeygenSideloadInsCnt = 573800,
kModeEcdhInsCnt = 581598,
kModeEcdhSideloadInsCnt = 581658,
kModeEcdsaSignInsCnt = 607087,
kModeEcdsaSignSideloadInsCnt = 607147,
};

static status_t p256_masked_scalar_write(p256_masked_scalar_t *src,
Expand Down
2 changes: 1 addition & 1 deletion sw/otbn/crypto/boot.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ attestation_keygen:
The check fails if both sides are not equal.
FG0.Z <= (y^2) mod p == (x^2 + ax + b) mod p */
bn.cmp w18, w19
jal x1, trigger_fault_if_fg0_not_z
jal x1, trigger_fault_if_fg0_z

ecall

Expand Down
55 changes: 24 additions & 31 deletions sw/otbn/crypto/p256_base.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,33 +43,27 @@
* sensitive; since aborting the program will be quicker than completing it,
* the flag's value is likely clearly visible to an attacker through timing.
*
* @param[in] w31: all-zero
* @param[in] FG0.Z: boolean indicating fault condition
* @param[in] FG0.Z: boolean indicating fault condition when 1
Copy link
Contributor

@vogelpi vogelpi Oct 27, 2025

Choose a reason for hiding this comment

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

Also for this file, can you please check the function names and the header comments please? In particular:

  • Line 36 says this function triggers a fault if fault if FG0.Z is 1
    The function is then named trigger_fault_if_fg0_z (does the final z stand for the Z of the flag or for zero?)
    Line 60 is aligned with that (cause error if FG0.Z is 1)
  • Line 71 says the second function triggers a fault if FG0.Z is 0
    The function is named trigger_fault_if_fg0_not_z - I think this name is counterintuitive comparing it with the terminology / function name above
    Line 96 then again says the error is triggered if FG0.Z is 0.

Maybe the counterintuitive function name is what confused me also with the other comment still open. Can we please align things?

Copy link
Contributor Author

@h-filali h-filali Oct 27, 2025

Choose a reason for hiding this comment

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

If fg0_z means fg0 is zero, then the naming is correct. However it can clearly be interpreted differently. I also thought the function names should be changed but I didn't want to make this change about changing the namings. I'll change the naming in my next amendment.

*
* clobbered registers: x2
* clobbered registers: x2, w31
* clobbered flag groups: none
*/
trigger_fault_if_fg0_z:
trigger_fault_if_fg0_not_z:
/* Read the FG0.Z flag (position 3).
x2 <= FG0.Z */
csrrw x2, FG0, x0
andi x2, x2, 8
srli x2, x2, 3
addi x2, x2, 31

/* Subtract FG0.Z from 0.
x2 <= 0 - x2 = FG0.Z ? 2^32 - 1 : 0 */
sub x2, x0, x2

/* The `bn.lid` instruction causes an `BAD_DATA_ADDR` error if the
memory address is out of bounds. Therefore, if FG0.Z is 1, this
instruction causes an error, but if FG0.Z is 0 it simply loads the word at
address 0 into w31. */
li x3, 31
bn.lid x3, 0(x2)
/* The `bn.lid` instruction causes an `ILLEGAL_INSN` error if the index of the
bignum register (stored in x2 in this case) is invalid. Therefore, if FG0.Z
is 1, this instruction causes an error, but if FG0.Z is 0 it simply loads
the word at address 0 into w31. */
bn.lid x2, 0(x0)

/* If we get here, the flag must have been 0. Restore w31 to zero and return.
w31 <= 0 */
bn.xor w31, w31, w31
bn.xor w31, w31, w31

ret

Expand All @@ -84,29 +78,28 @@ trigger_fault_if_fg0_z:
* sensitive; since aborting the program will be quicker than completing it,
* the flag's value is likely clearly visible to an attacker through timing.
*
* @param[in] w31: all-zero
* @param[in] FG0.Z: boolean indicating fault condition
* @param[in] FG0.Z: boolean indicating fault condition when 0
*
* clobbered registers: x2
* clobbered registers: x2, w31
* clobbered flag groups: none
*/
trigger_fault_if_fg0_not_z:
trigger_fault_if_fg0_z:
/* Read the FG0.Z flag (position 3).
x2 <= FG0.Z */
csrrw x2, FG0, x0
andi x2, x2, 8
slli x2, x2, 3
xori x2, x2, 8
addi x2, x2, 31

/* The `bn.lid` instruction causes an `BAD_DATA_ADDR` error if the
memory address is out of bounds. Therefore, if FG0.Z is 1, this
instruction causes an error, but if FG0.Z is 0 it simply loads the word at
address 0 into w31. */
li x3, 31
bn.lid x3, 0(x2)
/* The `bn.lid` instruction causes an `ILLEGAL_INSN` error if the index of the
bignum register (stored in x2 in this case) is invalid. Therefore, if FG0.Z
is 0, this instruction causes an error, but if FG0.Z is 1 it simply loads
the word at address 0 into w31. */
bn.lid x2, 0(x0)

/* If we get here, the flag must have been 1. Restore w31 to zero and return.
w31 <= 0 */
bn.xor w31, w31, w31
bn.xor w31, w31, w31

ret

Expand Down Expand Up @@ -1358,7 +1351,7 @@ scalar_mult_int:
bn.rshi w2, w2, w20 >> 65

/* double-and-add loop with decreasing index */
loopi 321, 64
loopi 321, 63

/* double point Q
Q = (w8, w9, w10) <= 2*(w8, w9, w10) = 2*Q */
Expand Down Expand Up @@ -1517,7 +1510,7 @@ scalar_mult_int:

FG0.Z <= if (w10 == 0) then 1 else 0 */
bn.cmp w10, w31
jal x0, trigger_fault_if_fg0_z
jal x0, trigger_fault_if_fg0_not_z


/**
Expand Down Expand Up @@ -1608,7 +1601,7 @@ p256_base_mult:
The check fails if both sides are not equal.
FG0.Z <= (y^2) mod p == (x^2 + ax + b) mod p */
bn.cmp w18, w19
jal x1, trigger_fault_if_fg0_not_z
jal x1, trigger_fault_if_fg0_z

ret

Expand Down
2 changes: 1 addition & 1 deletion sw/otbn/crypto/p256_shared_key.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ p256_shared_key:
The check fails if both sides are not equal.
FG0.Z <= (y^2) mod p == (x^2 + ax + b) mod p */
bn.cmp w18, w19
jal x1, trigger_fault_if_fg0_not_z
jal x1, trigger_fault_if_fg0_z

/* Arithmetic masking:
1. Generate a random mask
Expand Down
4 changes: 2 additions & 2 deletions sw/otbn/crypto/p256_sign.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ p256_sign:
The check fails if both sides are not equal.
FG0.Z <= (y^2) mod p == (x^2 + ax + b) mod p */
bn.cmp w18, w19
jal x1, trigger_fault_if_fg0_not_z
jal x1, trigger_fault_if_fg0_z

/* setup modulus n (curve order) and Barrett constant
MOD <= w29 <= n = dmem[p256_n]; w28 <= u_n = dmem[p256_u_n] */
Expand Down Expand Up @@ -288,7 +288,7 @@ p256_sign:
which violates ECDSA private key requirements. This could technically be
triggered by an unlucky key manager seed, but the probability is so low (~1/n)
that it more likely indicates a fault attack. */
jal x1, trigger_fault_if_fg0_z
jal x1, trigger_fault_if_fg0_not_z

/* w24 = r <= w11 mod n */
bn.addm w24, w11, w31
Expand Down
29 changes: 14 additions & 15 deletions sw/otbn/crypto/p384_isoncurve.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,30 +27,29 @@
* sensitive; since aborting the program will be quicker than completing it,
* the flag's value is likely clearly visible to an attacker through timing.
*
* @param[in] w31: all-zero
* @param[in] FG0.Z: boolean indicating fault condition
* @param[in] FG0.Z: boolean indicating fault condition when 0
*
* clobbered registers: x2, x3
* clobbered registers: x2, w31
* clobbered flag groups: none
*/
.globl trigger_fault_if_fg0_not_z
trigger_fault_if_fg0_not_z:
.globl trigger_fault_if_fg0_z
trigger_fault_if_fg0_z:
/* Read the FG0.Z flag (position 3).
x2 <= FG0.Z */
csrrw x2, FG0, x0
andi x2, x2, 8
slli x2, x2, 3
xori x2, x2, 8
addi x2, x2, 31

/* The `bn.lid` instruction causes an `BAD_DATA_ADDR` error if the
memory address is out of bounds. Therefore, if FG0.Z is 1, this
instruction causes an error, but if FG0.Z is 0 it simply loads the word at
address 0 into w31. */
li x3, 31
bn.lid x3, 0(x2)
/* The `bn.lid` instruction causes an `ILLEGAL_INSN` error if the index of the
bignum register (stored in x2 in this case) is invalid. Therefore, if FG0.Z
is 1, this instruction causes an error, but if FG0.Z is 0 it simply loads
the word at address 0 into w31. */
bn.lid x2, 0(x0)

/* If we get here, the flag must have been 1. Restore w31 to zero and return.
w31 <= 0 */
bn.xor w31, w31, w31
bn.xor w31, w31, w31

ret

Expand Down Expand Up @@ -214,11 +213,11 @@ p384_isoncurve_check:
FG0.Z <= (y^2) mod p == (x^2 + ax + b) mod p */
bn.cmp w4, w6
/* Fail if FG0.Z is false. */
jal x1, trigger_fault_if_fg0_not_z
jal x1, trigger_fault_if_fg0_z

bn.cmp w5, w7
/* Fail if FG0.Z is false. */
jal x1, trigger_fault_if_fg0_not_z
jal x1, trigger_fault_if_fg0_z

ret

Expand Down
4 changes: 2 additions & 2 deletions sw/otbn/crypto/p384_isoncurve_proj.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ p384_isoncurve_proj_check:
FG0.Z <= (y^2) mod p == (x^2 + ax + b) mod p */
bn.cmp w4, w2
/* Fail if FG0.Z is false. */
jal x1, trigger_fault_if_fg0_not_z
jal x1, trigger_fault_if_fg0_z

bn.cmp w5, w3
/* Fail if FG0.Z is false. */
jal x1, trigger_fault_if_fg0_not_z
jal x1, trigger_fault_if_fg0_z

ret

Expand Down
9 changes: 9 additions & 0 deletions sw/otbn/crypto/tests/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,10 @@ otbn_sim_test(

otbn_consttime_test(
name = "p256_base_mult_consttime",
ignore = [
"trigger_fault_if_fg0_z",
"trigger_fault_if_fg0_not_z",
],
subroutine = "p256_base_mult",
deps = [
"//sw/otbn/crypto:run_p256",
Expand Down Expand Up @@ -275,6 +279,10 @@ otbn_sim_test(

otbn_consttime_test(
name = "p256_shared_key_consttime",
ignore = [
"trigger_fault_if_fg0_z",
"trigger_fault_if_fg0_not_z",
],
subroutine = "p256_shared_key",
deps = [
"//sw/otbn/crypto:run_p256",
Expand Down Expand Up @@ -674,6 +682,7 @@ otbn_consttime_test(
otbn_consttime_test(
name = "p384_scalar_mult_consttime",
ignore = [
"trigger_fault_if_fg0_z",
"trigger_fault_if_fg0_not_z",
],
subroutine = "p384_scalar_mult",
Expand Down
Loading