Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Turin TCB support #170

wants to merge 2 commits into from

Conversation

jraman567
Copy link

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

@jraman567 jraman567 requested a review from deeglaze June 17, 2025 15:38
Copy link

@larrydewey larrydewey left a 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

@jraman567 jraman567 requested review from deeglaze and larrydewey June 17, 2025 18:35
@jraman567
Copy link
Author

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>
@jraman567 jraman567 force-pushed the turin-tcb-v2 branch 2 times, most recently from 0160907 to 68e5829 Compare June 18, 2025 03:30
@jraman567 jraman567 requested a review from deeglaze June 18, 2025 03:34
@jraman567
Copy link
Author

@deeglaze Thanks for reviewing! I've addressed your comment to make the version fields private.

kds/kds.go Outdated
Comment on lines 72 to 75
// 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)
Copy link
Contributor

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.

@jraman567
Copy link
Author

@deeglaze I've addressed your recent comments

switch c.String(productLine) {
case "Turin":
return &TCBVersionStruct{version: tcbStructVersion1, TCB: tcb}, nil
default:
Copy link
Contributor

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.

Copy link
Author

@jraman567 jraman567 Jun 18, 2025

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?

Copy link
Contributor

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.

Copy link
Author

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

Copy link
Author

@jraman567 jraman567 Jun 18, 2025

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}},
		},

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Author

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.

Copy link
Contributor

@deeglaze deeglaze left a 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>
@jraman567 jraman567 requested a review from deeglaze June 18, 2025 23:11
@jraman567 jraman567 mentioned this pull request Jun 19, 2025
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.

3 participants