Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Formally propose a transport-agnostic Cerberus #16
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
base: master
Are you sure you want to change the base?
Formally propose a transport-agnostic Cerberus #16
Changes from all commits
4218020
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we also stick with the currently defined maximum of 4096 bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could. I'm not sure there's an immediate thing this restriction buys us (it certainly doesn't benefit the client in any way I can see...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe things change when you move to non-I2C/MTCP busses, but having a hard cap on the maximum message size is useful, especially when talking about hierarchical attestation. The PA-RoT knows if it allocates a message buffer of 4096 bytes, it will be able to fully communicate with any device. It also helps buffer sizing in the BMC to support routing messages to/from different devices within the system (e.g. PA-RoT <-> AC-RoT).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of other pieces of information in the current MCTP-based header:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the bit about Rq is just a detail of the transport... ultimately, Cerberus doesn't actually care about those other messages, so from its perspective they would never even show up; the transport implementation would route them to the correct firmware component on its own.
LMK if I'm missing something obvious.
Also, I've generalized the "is encrypted" bit to "is authenticated", since that's the main value of the bit. I'm mostly hand-waving the transport security away for now until I can figure out how to incorporate it in a future RFC. For now, the transport should be imagined as "knowing" the relevant shared secret (and ancillary information, such as an IV) for decrypting and replying to a message within a session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I certainly see the point about the Rq bit.
I'm not sure about abstracting the transport security, though. I think we'd have to do it in a way that leaves open support for the Cerberus protocol session establishment flow, which uses Cerberus protocol messages, so there seems to be some interdependence there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few items here around the Device Capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave the same message structure and semantics while achieving the goal of removing message sizes from the Challenge Protocol layer. It could be reworded to something like this:
"The amount of log information returned is determined by the actual log size or the maximum message size as determined by the protocol transport, whichever is smaller. If multiple messages are required to retrieve the full log contents, the end of log data is denoted by a response message with less than the transport-defined maximum number of bytes. A request for log data with the Offset field set to equal or greater than the log size will generate a response message of 0 bytes."
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this, but backwards compatibility in binary format is attractive... I want to sleep on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make sense to have a blanket statement at the start of Section 6 that includes my text from the previous context, but apply it to any command that takes an offset field and whose response could span multiple messages. I think that would address the issues in these other sections (aside from removing the 'MCTP' in some cases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our implementation is the following:
This Microsoft implementation seems to align with the suggested changes here, with a couple of minor modifications: