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

Closed
wants to merge 1 commit into from

Conversation

imphil
Copy link
Contributor

@imphil imphil commented Oct 13, 2021

Fixes #8502

@imphil
Copy link
Contributor Author

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?

Copy link

@cdgori cdgori left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

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).

Copy link
Contributor

@GregAC GregAC left a comment

Choose a reason for hiding this comment

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

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 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
Copy link
Contributor Author

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.

@vogelpi
Copy link
Contributor

vogelpi commented Jul 29, 2022

I am closing this PR as all this changes have been merged as part of #9350.

@vogelpi vogelpi closed this Jul 29, 2022
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.

[OTBN.H.KEY_SIDELOAD] Document/specify feature in OTBN documentation
6 participants