Skip to content

Conversation

nasahlpa
Copy link
Member

This PR improves the hardened_xor function.

In the previous version of this function, when passing two shares of a secret variable to it, one share was overwritten with the other share, which leaks.

The new function now performs:
dest = ((x ^ rand) ^ y) ^ rand

This looks like the following in assembly:

20014574:	418c                	lw	a1,0(a1)
20014576:	4118                	lw	a4,0(a0)
20014578:	8db9                	xor	a1,a1,a4
2001457a:	c28c                	sw	a1,0(a3)
2001457c:	4210                	lw	a2,0(a2)
2001457e:	8db1                	xor	a1,a1,a2
20014580:	c28c                	sw	a1,0(a3)
20014582:	4108                	lw	a0,0(a0)
20014584:	8d2d                	xor	a0,a0,a1
20014586:	c288                	sw	a0,0(a3)

The first commit in this PR belongs to #27984

@nasahlpa nasahlpa requested a review from a team as a code owner August 29, 2025 13:55
@nasahlpa nasahlpa requested review from timothytrippel and johannheyszl and removed request for a team and timothytrippel August 29, 2025 13:55
Copy link
Contributor

@johannheyszl johannheyszl left a comment

Choose a reason for hiding this comment

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

Thanks @nasahlpa this looks nice!

@nasahlpa nasahlpa added the CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 label Sep 1, 2025
@nasahlpa nasahlpa force-pushed the hardened_xor branch 2 times, most recently from 534e75f to e5c31ca Compare September 3, 2025 08:11
Copy link
Contributor

@h-filali h-filali left a comment

Choose a reason for hiding this comment

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

Thanks Pascal! I checked the last two commits. One share should not overwrite the other anymore. I also compared the loop to the existing hardened_memcpy implementation.
I also left two questions. But this looks good.

Rename this function to hardend_xor_in_place as the next commit
introduces a hardened_xor that does not store the result in-place.

Signed-off-by: Pascal Nasahl <[email protected]>
Takes `x` and `y` and writes `x ^ y` to the `dest` output buffer. To
avoid combining `x` and `y` in the XOR operation, the function
actually performs: `dest = ((rand ^ x) ^ y) ^ rand`

Closes lowRISC#28008

Signed-off-by: Pascal Nasahl <[email protected]>
With the previous hardend_xor implementation, we were overriding
share0 with share1, which leaks. By using the improved hardened_xor,
we avoiding this issue.

Signed-off-by: Pascal Nasahl <[email protected]>
@nasahlpa nasahlpa added this pull request to the merge queue Sep 16, 2025
Merged via the queue into lowRISC:master with commit 67a2111 Sep 16, 2025
46 checks passed
@lowrisc-ci
Copy link

lowrisc-ci bot commented Sep 16, 2025

Backport failed for earlgrey_1.0.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-28085-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-28085-to-earlgrey_1.0.0
git switch --create backport-28085-to-earlgrey_1.0.0
git cherry-pick -x 542beff0988afcfc65f8fb501beed186600a8544 54116843ab4dc031f04e6cfc3e2452fff6980a74 b126c26037688f186230539c47165d4f7fb6b9b3

@lowrisc-ci lowrisc-ci bot added the Manually CherryPick This PR should be manually cherry picked. label Sep 16, 2025
@lowrisc-ci
Copy link

lowrisc-ci bot commented Sep 16, 2025

Backport failed for earlgrey_1.0.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-28085-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-28085-to-earlgrey_1.0.0
git switch --create backport-28085-to-earlgrey_1.0.0
git cherry-pick -x 542beff0988afcfc65f8fb501beed186600a8544 54116843ab4dc031f04e6cfc3e2452fff6980a74 b126c26037688f186230539c47165d4f7fb6b9b3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 Manually CherryPick This PR should be manually cherry picked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants