Fix "modelstepping" handling in verification

The "Milan-B0" naming in a product name is not directly derivable from
the cpuid(1) values. Instead this B0 naming is from a different naming
convention I cannot yet determine. Despite this, I can at least overfit
the verification function for now so that if this model is off, tests
will fail loudly and we can fix it.

This fix has been verified in hardware tests.

Signed-off-by: Dionna Glaze <[email protected]>

abi/abi.go

@@ -823,6 +823,10 @@ func SevProduct() *pb.SevProduct {
 	// Ah, Fh, {0h,1h} values from the KDS specification,
 	// section "Determining the Product Name".
 	var productName pb.SevProduct_SevProductName
+	// Product information specified by processor programming reference publications.
+	// Milan-B0 specified as 00A00F10h
+	// Milan-B1 specified as 00A00F11h
+	// Genoa-A0 specified as 00A10F10h
 	if extendedFamily == 0xA && family == 0xF {
 		switch extendedModel {
 		case 0:
@@ -841,5 +845,5 @@ func SevProduct() *pb.SevProduct {
 
 // DefaultSevProduct returns the initial product version for a commercially available AMD SEV-SNP chip.
 func DefaultSevProduct() *pb.SevProduct {
-	return &pb.SevProduct{Name: pb.SevProduct_SEV_PRODUCT_MILAN, ModelStepping: 0xB0}
+	return &pb.SevProduct{Name: pb.SevProduct_SEV_PRODUCT_MILAN, ModelStepping: 0x11}
 }

kds/kds.go

@@ -692,22 +692,51 @@ func ProductName(product *pb.SevProduct) string {
 	if product == nil {
 		product = abi.DefaultSevProduct()
 	}
-	return fmt.Sprintf("%s-%02X", ProductString(product), product.ModelStepping)
+	model := (product.ModelStepping >> 4) & 0xf
+	if model != 1 {
+		return "Unknown"
+	}
+	stepping := product.ModelStepping & 0xf
+	// The string is not fully programmable, given the processor programming reference revisions.
+	switch product.Name {
+	case pb.SevProduct_SEV_PRODUCT_MILAN:
+		return fmt.Sprintf("Milan-B%X", stepping)
+	case pb.SevProduct_SEV_PRODUCT_GENOA:
+		return fmt.Sprintf("Genoa-A%X", stepping)
+	default:
+		return "Unknown"
+	}
+}
+
+func parseHexRune(r byte) (byte, error) {
+	if r >= '0' && r <= '9' {
+		return r - byte('0'), nil
+	}
+	if r >= 'A' && r <= 'F' {
+		return 10 + (r - byte('A')), nil
+	}
+	if r >= 'a' && r <= 'f' {
+		return 10 + (r - byte('a')), nil
+	}
+	return 0, fmt.Errorf("%q is not a hexadecimal character", rune(r))
 }
 
 // ParseProductName returns the KDS project input value, and the model, stepping numbers represented
 // by a given V[CL]EK productName extension value, or an error.
 func ParseProductName(productName string, key abi.ReportSigner) (*pb.SevProduct, error) {
-	var product, stepping string
+	var product, revisionStepping string
 	var needStepping bool
 	switch key {
 	case abi.VcekReportSigner:
 		subs := strings.SplitN(productName, "-", 2)
 		if len(subs) != 2 {
-			return nil, fmt.Errorf("productName value %q does not match the VCEK expected Name-ModelStepping format", productName)
+			return nil, fmt.Errorf("productName value %q does not match the VCEK expected Name-RevisionStepping format", productName)
 		}
 		product = subs[0]
-		stepping = subs[1]
+		revisionStepping = subs[1]
+		if len(revisionStepping) != 2 {
+			return nil, fmt.Errorf("productName post '-', %q is not 2 characters", revisionStepping)
+		}
 		needStepping = true
 	case abi.VlekReportSigner:
 		// VLEK certificates don't carry the stepping value in productName.
@@ -725,10 +754,16 @@ func ParseProductName(productName string, key abi.ReportSigner) (*pb.SevProduct,
 	var modelStepping uint64
 	if needStepping {
 		var err error
-		modelStepping, err = strconv.ParseUint(stepping, 16, 8)
+		stepping, err := parseHexRune(revisionStepping[1])
 		if err != nil {
-			return nil, fmt.Errorf("model stepping in productName is not a hexadecimal byte: %q", stepping)
+			return nil, fmt.Errorf("stepping in productName is not a hexadecimal byte: %q", stepping)
+		}
+		if product == "Milan" && revisionStepping[0] != byte('B') {
+			return nil, fmt.Errorf("Milan revision is %q, expected %q", revisionStepping[0], "B")
+		} else if product == "Genoa" && revisionStepping[0] != byte('A') {
+			return nil, fmt.Errorf("Genoa revision is %q, expected %q", revisionStepping[0], "A")
 		}
+		modelStepping = 0x10 | uint64(stepping)
 	}
 	return &pb.SevProduct{Name: name, ModelStepping: uint32(modelStepping)}, nil
 }

kds/kds_test.go

@@ -185,29 +185,29 @@ func TestProductName(t *testing.T) {
 	}{
 		{
 			name: "nil",
-			want: "Milan-B0",
+			want: "Milan-B1",
 		},
 		{
 			name: "unknown",
 			input: &pb.SevProduct{
 				ModelStepping: 0x1A,
 			},
-			want: "Unknown-1A",
+			want: "Unknown",
 		},
 		{
 			name: "Milan-00",
 			input: &pb.SevProduct{
 				Name: pb.SevProduct_SEV_PRODUCT_MILAN,
 			},
-			want: "Milan-00",
+			want: "Unknown",
 		},
 		{
 			name: "Genoa-FF",
 			input: &pb.SevProduct{
 				Name:          pb.SevProduct_SEV_PRODUCT_GENOA,
 				ModelStepping: 0xFF,
 			},
-			want: "Genoa-FF",
+			want: "Unknown",
 		},
 	}
 	for _, tc := range tcs {
@@ -234,7 +234,7 @@ func TestParseProductName(t *testing.T) {
 		{
 			name:    "Too much",
 			input:   "Milan-B0-and some extra",
-			wantErr: "not a hexadecimal byte: \"B0-and some extra\"",
+			wantErr: "productName post '-', \"B0-and some extra\" is not 2 characters",
 		},
 		{
 			name:    "start-",
@@ -244,21 +244,31 @@ func TestParseProductName(t *testing.T) {
 		{
 			name:    "end-",
 			input:   "Milan-",
-			wantErr: "model stepping in productName is not a hexadecimal byte: \"\"",
+			wantErr: "not 2 characters",
 		},
 		{
 			name:    "Too big",
 			input:   "Milan-100",
-			wantErr: "model stepping in productName is not a hexadecimal byte: \"100\"",
+			wantErr: "productName post '-', \"100\" is not 2 characters",
 		},
 		{
-			name:  "happy path",
-			input: "Genoa-9C",
+			name:  "happy path Genoa",
+			input: "Genoa-A1",
 			want: &pb.SevProduct{
 				Name:          pb.SevProduct_SEV_PRODUCT_GENOA,
-				ModelStepping: 0x9C,
+				ModelStepping: 0x11,
 			},
 		},
+		{
+			name:    "bad revision Genoa",
+			input:   "Genoa-B1",
+			wantErr: "Genoa revision is 'B', expected \"A\"",
+		},
+		{
+			name:    "bad revision Milan",
+			input:   "Milan-A1",
+			wantErr: "Milan revision is 'A', expected \"B\"",
+		},
 		{
 			name:  "vlek products have no stepping",
 			input: "Genoa",

testing/mocks.go

@@ -145,7 +145,7 @@ func (d *Device) Product() *spb.SevProduct {
 	if d.SevProduct == nil {
 		return &spb.SevProduct{
 			Name:          spb.SevProduct_SEV_PRODUCT_MILAN,
-			ModelStepping: 0xB0,
+			ModelStepping: 0x10,
 		}
 	}
 	return d.SevProduct

verify/verify.go

@@ -650,15 +650,18 @@ func SnpAttestation(attestation *spb.Attestation, options *Options) error {
 // fillInAttestation uses AMD's KDS to populate any empty certificate field in the attestation's
 // certificate chain.
 func fillInAttestation(attestation *spb.Attestation, options *Options) error {
+	var productOverridden bool
 	if options.Product != nil {
 		attestation.Product = options.Product
-	}
-	if attestation.Product == nil {
-		// The default product is the first launched SEV-SNP product value.
+		productOverridden = true
+	} else if attestation.Product == nil {
+		// The default product is the first launched SEV-SNP product value that isn't
+		// an engineering preview.
 		attestation.Product = &spb.SevProduct{
 			Name:          spb.SevProduct_SEV_PRODUCT_MILAN,
-			ModelStepping: 0xB0,
+			ModelStepping: 0x11,
 		}
+		productOverridden = true
 	}
 	if options.DisableCertFetching {
 		return nil
@@ -702,6 +705,21 @@ func fillInAttestation(attestation *spb.Attestation, options *Options) error {
 				}
 			}
 			chain.VcekCert = vcek
+			if productOverridden {
+				cert, err := x509.ParseCertificate(vcek)
+				if err != nil {
+					return err
+				}
+				exts, err := kds.VcekCertificateExtensions(cert)
+				if err != nil {
+					return err
+				}
+				attestation.Product, err = kds.ParseProductName(exts.ProductName, abi.VcekReportSigner)
+				fmt.Printf("filled in product with %v\n", attestation.Product)
+				if err != nil {
+					return err
+				}
+			}
 		}
 	case abi.VlekReportSigner:
 		// We can't lazily ask KDS for the certificate as a user. The CSP must cache their provisioned