-
Notifications
You must be signed in to change notification settings - Fork 26
Turin TCB support #170
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: main
Are you sure you want to change the base?
Turin TCB support #170
Conversation
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.
Some comments and questions
Thank you, @deeglaze and @larrydewey for reviewing this PR. :) I've addressed your comments. |
Turin and later families have FMC SPL in their TCBs. Additionally, the HwID in the TCB is only 8 bytes long for Turin and later, compared to 64 bytes or Genoa and earlier. Please see section 3.1 in the spec below: https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/57230.pdf Signed-off-by: Jagannathan Raman <jraman567@gmail.com>
0160907
to
68e5829
Compare
@deeglaze Thanks for reviewing! I've addressed your comment to make the version fields private. |
kds/kds.go
Outdated
// TCBStructVersion0 is the version of the TCB structure for Milan & Genoa | ||
TCBStructVersion0 = uint8(0) | ||
// TCBStructVersion1 is the version of the TCB structure for Turin | ||
TCBStructVersion1 = uint8(1) |
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.
Struct version constants shouldn't need to be exported given the version fields aren't exported.
@deeglaze I've addressed your recent comments |
switch c.String(productLine) { | ||
case "Turin": | ||
return &TCBVersionStruct{version: tcbStructVersion1, TCB: tcb}, nil | ||
default: |
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.
This should fail closed. We don't want "Venice" to accidentally end up misinterpreted here.
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.
@deeglaze When I do that, some of the validate tests are failing. The root cause is that the Reports generated by the QuoteProvider don't have the FMS set. Do you know if we could configure the QuoteProvider to fill the FMS in the report?
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.
You'll need to change snpReportVersion in validate_test.go to 3 before you do that, but I'd also like to know what the product is that it think's it's 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.
The FMS is 0. So, if we decoded the productLine from FMS, it is "Unknown".
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.
The issue is the report generated in the following code doesn't have FMS in it:
{
name: "just reportData",
attestation: func() *spb.Attestation {
q, err := sg.GetQuoteProto(qp0, nonce0s1)
if err != nil {
t.Fatal(err)
}
report := q.Report
//report.Cpuid1EaxFms = 0xa00f11
return &spb.Attestation{
Report: report,
CertificateChain: &spb.CertificateChain{
AskCert: sign0.Ask.Raw,
ArkCert: sign0.Ark.Raw,
VcekCert: sign0.Vcek.Raw,
},
}
}(),
opts: &Options{ReportData: nonce0s1[:], GuestPolicy: abi.SnpPolicy{Debug: true}},
},
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.
Yes, we can use test data that depends on version 3 and populates the FMS.
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.
Thanks, @deeglaze ! The tests in the "tools/check" directory are failing too. The reports that they generate are v2, which don't have FMS. So, we're unable to decode the product family from them.
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 could add file and track an issue to use v3 reports in tools/check and validate_test, if you're OK with defaulting to structVersion0 here.
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.
LGTM with final comments.
- fix lint errors reported by Ubuntu CI - make error messages in tests reader friendly - address Larry's feedback - address backwards compatibility issues raised in the review Signed-off-by: Jagannathan Raman <jraman567@gmail.com>
Turin and later families have FMC SPL in their TCBs. Additionally, the HwID in the TCB is only 8 bytes long for Turin and later, compared to 64 bytes or Genoa and earlier.
Please see section 3.1 in the spec below:
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/57230.pdf