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

[otbn] Document key sideload #8650

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

Conversation

imphil
Copy link
Contributor

@imphil imphil commented Oct 13, 2021

Fixes #8502

@imphil
Copy link
Contributor Author

@imphil imphil commented Oct 13, 2021

This PR won't pass CI without further changes to the system, but I'd like to focus on the functionality first.

This spec proposal writes down what we have discussed earlier, with two deviations:

  • I realized the key manager provides the key in two shares: I hence added two sets of key WSRs, one for each share.
  • We somehow need to expose the valid bit. I chose to raise a software error (recoverable by default) instead of returning dummy data (e.g. 0) to make it easier to detect if somehow the key data (due to a programmer error or an attack) is invalid.

Note that we don't integrity-protect the key data between the key manager and OTBN at the moment, but we also don't plan to register it in OTBN at all, but to simply pass through the "wires" from key manager.

@tjaychen @cdgori @felixmiller Is this design what you were looking for?

cdgori
cdgori approved these changes Oct 13, 2021
Copy link

@cdgori cdgori left a comment

The error approach seems fine to me.

It's been so long that I don't remember the specific use case we anticipated here, maybe @felixmiller or @tjaychen remembers?

I'm guessing this is to connect an ECDSA p-384 private key for signing. (Presumably it would also work with p-256, just ignoring the upper bits?)

(But I would think that the case of ECDH to generate a session key would require communication in the other direction, maybe we decided that was out-of-scope here? The sideload mechanism as currently spec'd doesn't really work like that, I guess.)

Copy link
Contributor

@rswarbrick rswarbrick left a comment

Looks good (and nice and simple) to me.

I think we should probably update the block diagram to show the connection though.

@tjaychen
Copy link
Contributor

@tjaychen tjaychen commented Oct 14, 2021

This PR won't pass CI without further changes to the system, but I'd like to focus on the functionality first.

This spec proposal writes down what we have discussed earlier, with two deviations:

  • I realized the key manager provides the key in two shares: I hence added two sets of key WSRs, one for each share.
  • We somehow need to expose the valid bit. I chose to raise a software error (recoverable by default) instead of returning dummy data (e.g. 0) to make it easier to detect if somehow the key data (due to a programmer error or an attack) is invalid.

Note that we don't integrity-protect the key data between the key manager and OTBN at the moment, but we also don't plan to register it in OTBN at all, but to simply pass through the "wires" from key manager.

@tjaychen @cdgori @felixmiller Is this design what you were looking for?

yes that sounds good to me. I think the idea behind not integrity protecting it is because it's assumed there will be software redundancy (all the way to the keymgr level).

GregAC
GregAC approved these changes Oct 15, 2021
Copy link
Contributor

@GregAC GregAC left a comment

Just to check my understanding the sideload is dealt with externally to OTBN core and just provides a single set of keys that the new WSRs read from and the bits read remain the same throughout the OTBN operation?

hw/ip/otbn/doc/_index.md Outdated Show resolved Hide resolved
@imphil
Copy link
Contributor Author

@imphil imphil commented Oct 15, 2021

Just to check my understanding the sideload is dealt with externally to OTBN core and just provides a single set of keys that the new WSRs read from

Yes.

and the bits read remain the same throughout the OTBN operation?

That's not guaranteed by hardware, but must be taken care of by the software author. Host software can reconfigure the key manager during an OTBN operation to change the keys. However, that wouldn't be wise, as there's no way for host software to know when OTBN actually uses the key it was provided with.

Also include the new connection to the Key Manger in the block diagram.

Fixes lowRISC#8502

Signed-off-by: Philipp Wagner <phw@lowrisc.org>
@imphil imphil force-pushed the otbn-keymgr-connection branch from e9f0450 to a7cd523 Compare Oct 15, 2021
@imphil
Copy link
Contributor Author

@imphil imphil commented Oct 15, 2021

I think we should probably update the block diagram to show the connection though.

done.

I've also specified that the higher bits of the second part of the key read as zero, and that the actual key data is in the lower 128b.

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.

5 participants