-
Notifications
You must be signed in to change notification settings - Fork 26
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
Formally propose a transport-agnostic Cerberus #16
base: master
Are you sure you want to change the base?
Conversation
transport layer is admissible. | ||
|
||
- The current contents of Chapter 3 should be moved into an appendix, provided | ||
as an example admissible transport layer for I2C connections. The same should |
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.
s/transport layer for I2C connections/transport layer for SMBus/MCTP connections/g ?
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.
Well, no. MCTP is the transport, and I2C is the bus it runs on.
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.
SMBus would be the bus though
2. Each endpoint has a known *maximum message length*, measured in bytes. The | ||
transport is responsible for negotiating this parameter for each device a | ||
Cerberus device intends to speak to. This parameter must be made available | ||
to the Cerberus protocol but Cerberus itself does not negotiate it. This |
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).
parameters. How they are encoded is irrelevant to Cerberus, beyond that they | ||
be computable without recieving the entire incoming message: | ||
- The command type byte (as specified in the Challenge Protocol). | ||
- A single bit: whther this message is the request or response half of the |
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.
Is it necessary to call out 'a single bit' here? Seems to contradict the previous statement of encoding being irrelevant.
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 "a boolean" is probably more appropriate?
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.
Boolean probably works, but it feels like you don't even need to be that specific. Simply stating that the transport should understand whether the message is request or response seems sufficient. Completely stays out of implementation details.
command. | ||
- The length of the incoming message, in bytes. | ||
- Addressing information that determines which device sent the message | ||
(the actual contents of this information is irrelevant to Cerberus). |
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:
- An indication if the payload is encrypted or not. We would need to carry this information into the new transport header.
- The 'Rq' bit that can be used to provide additional routing for message handling. Maybe it would enough to say that we have a mechanism to identify Cerberus commands from other device commands over the same transport.
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.
- Mentions of MCTP in Section 6.7 should be removed. The maximum message/packet | ||
size fields should be removed, since the transport layer should negotiate | ||
these while establishing a session, and pass the maximum message size field | ||
along to Cerberus. |
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.
- I think we are going to need to tweak this command in more ways than just this packet size piece (e.g. we don't want devices to use less than 256-bit ECC and we need to support higher bit lengths). For compatibility reasons, it probably would make sense to deprecate the existing capabilities command and define a new one. We can remove the old one from the spec at some point in the future.
- What are your thoughts around command timeouts? The timeouts are very important from an I2C point of view, especially in a multi-master configuration. I'm not sure they have the same value for other transports. Do you think command timeout would be something else that should be pushed to the transport layer? In the current Cerberus code, the timeout is only consumed in the cmd_channel, well out of the command processing scope. The command processing informs of the timeout to apply (normal vs. crypto), but doesn't actually enforce anything.
- We would need to define a new command in the MCTP binding that can be used to discover packet sizes and timeouts. Would that go in the MCTP Appendix of this spec? If not, where would that get defined?
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.
- How about we keep the message, but deprecate the fields in this RFC; we can start working out the newer, shinier capabilities message another time.
- I don't have strong thoughts. I'm going to put it on the meeting agenda to discuss when we get the opportunity. Perhaps this is just something that the transport should negotiate, too?
- I defer to you here, since you probably know MCTP a bit better. I would be fine to add a new command at the MCTP level. (I have a general expectation that all transports will do some kind of negotiation step like this.)
reference to payload sizes, we recommend that, instead of replying with | ||
multiple messages of maximum size, we add a "continue" bit to the Get Log | ||
response. When this bit is set, the client must send another request, with | ||
the offset field increased by the length of the Get Log log contents field. |
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.
the offset field increased by the length of the Get Log log contents field. | ||
|
||
- The same applies to Sections 6.20, 6.23, and 6.31. | ||
|
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).
As the author understands it, this is somewhat close to how the Microsoft | ||
implementation handles messages, although it calls the MCTP library directly | ||
rather than virtually. | ||
|
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:
- cmd_channel.receive_packet is called to get a packet of data from the transport (we are agnostic to the physical layer).
- When a packet is received, it is passed to the the MCTP stack through mctp_interface_process_packet. As you say, this is a direct call instead of a virtual one.
- The MCTP stack will parse the packet and work to reconstruct the complete message.
- If the current packet completes an MCTP message, the message (stripped of MCTP packet headers but not the MCTP vendor-specific command header) is sent for command processing through cmd_interface.process_request.
- If the message was an MCTP control message instead of a Cerberus one, it would be handled in the MCTP stack. MCTP errors are also directly handled by the MCTP stack.
- The response from command processing is packetized by the MCTP stack and returned to the cmd_channel for transmission.
- cmd_channel will loop through the list of packets and send them using cmd_channel.send_packet. There is no understanding of MCTP structure in the cmd_channel.
This Microsoft implementation seems to align with the suggested changes here, with a couple of minor modifications:
- Make the transport protocol stack a function pointer call instead of a direct call into MCTP handling.
- Add vender-specific commands to the MCTP stack to support message/packet size and timeout detection.
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 went through the RFC and made several comments. Let me know your thoughts.
Chris: I've replied to your comments and fixed up the text in accordance to them. Still a few open questions; I dropped items in the agenda that we can get to at some point. |
Signed-off-by: Miguel Young de la Sota <[email protected]>
After much dragging my feet I've put the proposal into RFC. I look forward to hearing feedback here. =)