From 0afe43ac9f65b9c0d3c5781c92a579b44308200c Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 15 Sep 2025 15:07:07 +0100 Subject: [PATCH 1/8] Validate host data Signed-off-by: Takuro Sato --- cmd/gcs-sidecar/main.go | 24 ++- internal/gcs-sidecar/handlers.go | 2 +- internal/gcs-sidecar/host.go | 20 +- internal/pspdriver/pspdriver.go | 315 +++++++++++++++++++++++++++++++ 4 files changed, 356 insertions(+), 5 deletions(-) create mode 100644 internal/pspdriver/pspdriver.go diff --git a/cmd/gcs-sidecar/main.go b/cmd/gcs-sidecar/main.go index 4cd0d70b34..7e9a3ff865 100644 --- a/cmd/gcs-sidecar/main.go +++ b/cmd/gcs-sidecar/main.go @@ -15,6 +15,7 @@ import ( "github.com/Microsoft/hcsshim/internal/gcs/prot" shimlog "github.com/Microsoft/hcsshim/internal/log" "github.com/Microsoft/hcsshim/internal/oc" + "github.com/Microsoft/hcsshim/internal/pspdriver" "github.com/Microsoft/hcsshim/pkg/securitypolicy" "github.com/sirupsen/logrus" "go.opencensus.io/trace" @@ -214,13 +215,30 @@ func main() { return } + var snpMode = false + if err := pspdriver.StartPSPDriver(ctx); err != nil { + // When error happens, pspdriver.GetPspDriverError() returns true. + // When it happens, gcs-sidecar should keep the initial "deny" policy + // and reject all requests from the host. + logrus.WithError(err).Errorf("failed to start PSP driver") + } else { + snpMode, err = pspdriver.IsSNPMode(ctx) + if err != nil { + logrus.WithError(err).Errorf("failed to check SNP mode: %v", err) + } + logrus.Tracef("SNP mode: %v", snpMode) + } + // gcs-sidecar can be used for non-confidentail hyperv wcow // as well. So we do not always want to check for initialPolicyStance var initialEnforcer securitypolicy.SecurityPolicyEnforcer - // TODO (kiashok/Mahati): The initialPolicyStance is set to allow - // only for dev. This will eventually be set to allow/deny depending on - // on whether SNP is supported or not. initialPolicyStance := "allow" + if pspdriver.GetPspDriverError() != nil || snpMode { + // If the driver failed to start, policy should keep returning "deny" for anything. + // For SNP environment, the initial policy is "deny" but it will be updated + // per the user's security policy. + initialPolicyStance = "deny" + } switch initialPolicyStance { case "allow": initialEnforcer = &securitypolicy.OpenDoorSecurityPolicyEnforcer{} diff --git a/internal/gcs-sidecar/handlers.go b/internal/gcs-sidecar/handlers.go index 689e02bb66..9dc0818d31 100644 --- a/internal/gcs-sidecar/handlers.go +++ b/internal/gcs-sidecar/handlers.go @@ -323,7 +323,7 @@ func (b *Bridge) modifySettings(req *request) (err error) { case guestresource.ResourceTypeSecurityPolicy: securityPolicyRequest := modifyGuestSettingsRequest.Settings.(*guestresource.WCOWConfidentialOptions) log.G(ctx).Tracef("WCOWConfidentialOptions: { %v}", securityPolicyRequest) - _ = b.hostState.SetWCOWConfidentialUVMOptions(securityPolicyRequest) + _ = b.hostState.SetWCOWConfidentialUVMOptions(ctx, securityPolicyRequest) // Send response back to shim resp := &prot.ResponseBase{ diff --git a/internal/gcs-sidecar/host.go b/internal/gcs-sidecar/host.go index b17cd38448..03979ee31c 100644 --- a/internal/gcs-sidecar/host.go +++ b/internal/gcs-sidecar/host.go @@ -4,11 +4,13 @@ package bridge import ( + "context" "errors" "fmt" "sync" "github.com/Microsoft/hcsshim/internal/protocol/guestresource" + "github.com/Microsoft/hcsshim/internal/pspdriver" "github.com/Microsoft/hcsshim/pkg/securitypolicy" ) @@ -32,7 +34,7 @@ func NewHost(initialEnforcer securitypolicy.SecurityPolicyEnforcer) *Host { } } -func (h *Host) SetWCOWConfidentialUVMOptions(securityPolicyRequest *guestresource.WCOWConfidentialOptions) error { +func (h *Host) SetWCOWConfidentialUVMOptions(ctx context.Context, securityPolicyRequest *guestresource.WCOWConfidentialOptions) error { h.policyMutex.Lock() defer h.policyMutex.Unlock() @@ -40,6 +42,22 @@ func (h *Host) SetWCOWConfidentialUVMOptions(securityPolicyRequest *guestresourc return errors.New("security policy has already been set") } + if pspdriver.GetPspDriverError() != nil { + // For this case gcs-sidecar will keep initial deny policy. + return fmt.Errorf("occurred error while using PSP driver: %v", pspdriver.GetPspDriverError()) + } + + // Fetch report and validate host_data + hostData, err := securitypolicy.NewSecurityPolicyDigest(securityPolicyRequest.EncodedSecurityPolicy) + if err != nil { + return err + } + + if err := pspdriver.ValidateHostData(ctx, hostData[:]); err != nil { + // For this case gcs-sidecar will keep initial deny policy. + return err + } + // This limit ensures messages are below the character truncation limit that // can be imposed by an orchestrator maxErrorMessageLength := 3 * 1024 diff --git a/internal/pspdriver/pspdriver.go b/internal/pspdriver/pspdriver.go new file mode 100644 index 0000000000..cd2b07836f --- /dev/null +++ b/internal/pspdriver/pspdriver.go @@ -0,0 +1,315 @@ +//go:build windows +// +build windows + +package pspdriver + +import ( + "bytes" + "context" + "encoding/binary" + "encoding/hex" + "fmt" + "os" + "time" + "unsafe" + + "github.com/Microsoft/hcsshim/internal/log" + "github.com/pkg/errors" + "golang.org/x/sys/windows" + "golang.org/x/sys/windows/svc" + "golang.org/x/sys/windows/svc/mgr" +) + +const ( + serviceName = "AmdSnpPsp" +) + +const ( + SNPPSP_API_STATUS_SUCCESS = 0x00000000 + SNPPSP_API_STATUS_UNSUCCESSFUL = 0x00000001 + SNPPSP_API_STATUS_DRIVER_UNSUCCESSFUL = 0x00000003 + SNPPSP_API_STATUS_PSP_UNSUCCESSFUL = 0x00000004 + SNPPSP_API_STATUS_INVALID_PARAMETER = 0x00000005 + SNPPSP_API_STATUS_DEVICE_NOT_AVAILABLE = 0x00000006 +) + +// TODO: Fix duplication with pkg/amdsevsnp and merge this into it. + +const ( + SNPPSP_API_REPORT_DATA_SIZE = 64 + SNPPSP_API_REPORT_HOST_DATA_SIZE = 32 + SNPPSP_API_ATTESTATION_REPORT_SIZE = 0x4A0 +) + +type SNPPSPGuestRequestResult struct { + DriverStatus uint32 + PspStatus uint64 +} + +type report struct { + Version uint32 + GuestSVN uint32 + Policy uint64 + FamilyID [16]byte + ImageID [16]byte + VMPL uint32 + SignatureAlgo uint32 + PlatformVersion uint64 + PlatformInfo uint64 + AuthorKeyEn uint32 + Reserved1 uint32 + ReportData [SNPPSP_API_REPORT_DATA_SIZE]byte + Measurement [48]byte + HostData [SNPPSP_API_REPORT_HOST_DATA_SIZE]byte + IDKeyDigest [48]byte + AuthorKeyDigest [48]byte + ReportID [32]byte + ReportIDMA [32]byte + ReportTCB uint64 + Reserved2 [24]byte + ChipID [64]byte + CommittedSVN [8]byte + CommittedVersion [8]byte + LaunchSVN [8]byte + Reserved3 [168]byte + Signature [512]byte +} + +// Report represents parsed attestation report. +type Report struct { + Version uint32 + GuestSVN uint32 + Policy uint64 + FamilyID string + ImageID string + VMPL uint32 + SignatureAlgo uint32 + PlatformVersion uint64 + PlatformInfo uint64 + AuthorKeyEn uint32 + ReportData string + Measurement string + HostData []byte + IDKeyDigest string + AuthorKeyDigest string + ReportID string + ReportIDMA string + ReportTCB uint64 + ChipID string + CommittedSVN string + CommittedVersion string + LaunchSVN string + Signature string +} + +func (sr *report) report() Report { + return Report{ + Version: sr.Version, + GuestSVN: sr.GuestSVN, + Policy: sr.Policy, + FamilyID: hex.EncodeToString(mirrorBytes(sr.FamilyID[:])[:]), + ImageID: hex.EncodeToString(mirrorBytes(sr.ImageID[:])[:]), + VMPL: sr.VMPL, + SignatureAlgo: sr.SignatureAlgo, + PlatformVersion: sr.PlatformVersion, + PlatformInfo: sr.PlatformInfo, + AuthorKeyEn: sr.AuthorKeyEn, + ReportData: hex.EncodeToString(sr.ReportData[:]), + Measurement: hex.EncodeToString(sr.Measurement[:]), + HostData: sr.HostData[:], + IDKeyDigest: hex.EncodeToString(sr.IDKeyDigest[:]), + AuthorKeyDigest: hex.EncodeToString(sr.AuthorKeyDigest[:]), + ReportID: hex.EncodeToString(sr.ReportID[:]), + ReportIDMA: hex.EncodeToString(sr.ReportIDMA[:]), + ReportTCB: sr.ReportTCB, + ChipID: hex.EncodeToString(sr.ChipID[:]), + CommittedSVN: hex.EncodeToString(sr.CommittedSVN[:]), + CommittedVersion: hex.EncodeToString(sr.CommittedVersion[:]), + LaunchSVN: hex.EncodeToString(sr.LaunchSVN[:]), + Signature: hex.EncodeToString(sr.Signature[:]), + } +} + +// mirrorBytes mirrors the byte ordering so that hex-encoding little endian +// ordered bytes come out in the readable order. +func mirrorBytes(b []byte) []byte { + for i := 0; i < len(b)/2; i++ { + mirrorIndex := len(b) - i - 1 + b[i], b[mirrorIndex] = b[mirrorIndex], b[i] + } + return b +} + +var ( + amdsnppspapi = windows.NewLazySystemDLL("amdsnppspapi.dll") + // It will panic if the function is not found when .Call() is called. + isSnpModeProc = amdsnppspapi.NewProc("SnpPspIsSnpMode") + fetchAttestationReportProc = amdsnppspapi.NewProc("SnpPspFetchAttestationReport") + pspDriverStarted = false + // The error needs to be stored to be retrieved later. + // When driver or its dll fails, gcs-sidecar doesn't + // set security policy and keep the initial deny policy. + pspDriverError error = nil +) + +// TODO: Should we use ctx for functions here? + +func StartPSPDriver(ctx context.Context) error { + // Connect to the Service Control Manager + m, err := mgr.Connect() + if err != nil { + return errors.Wrap(err, "Failed to connect to service manager") + } + defer m.Disconnect() + + // Open the service + s, err := m.OpenService(serviceName) + if err != nil { + return errors.Wrapf(err, "Could not access service %q", serviceName) + } + defer s.Close() + + // Start the service + err = s.Start() + if err != nil { + return errors.Wrapf(err, "Could not start service %q", serviceName) + } + + // From the documentation, there is no guarantee that the service will be + // in `Running` state immediately after starting it. + // Wait until the service is in the `Running` state. + timeout := time.After(3 * time.Second) + tick := time.Tick(100 * time.Millisecond) + for { + select { + case <-timeout: + pspDriverError = errors.New("timed out waiting for PSP driver to start") + return pspDriverError + case <-tick: + status, err := s.Query() + if err != nil { + pspDriverError = errors.Wrap(err, "could not query PSP driver status") + return pspDriverError + } + if status.State == svc.Running { + log.G(ctx).Tracef("Service %q started successfully", serviceName) + + pspDriverStarted = true + return nil + } + } + } +} + +func IsPspDriverStarted() bool { + return pspDriverStarted +} + +// Return an error from the PSP driver dll +// when it fails to use the dll at all. +// Otherwise it returns nil. +func GetPspDriverError() error { + return pspDriverError +} + +// IsSNPMode() returns true if it's in SNP mode. +func IsSNPMode(ctx context.Context) (bool, error) { + + if pspDriverError != nil { + return false, pspDriverError + } + + if !pspDriverStarted { + return false, errors.New("PSP driver is not started") + } + + // snpMode is defined as BOOLEAN (= byte) + var snpMode uint8 + ret, _, _ := isSnpModeProc.Call(uintptr(unsafe.Pointer(&snpMode))) + if ret != SNPPSP_API_STATUS_SUCCESS { + pspDriverError = errors.Errorf("failed to determine if it's in SNP VM. SNPPSP_API_STATUS: 0x%x", ret) + return false, pspDriverError + } + + return snpMode == 1, nil +} + +// FetchRawSNPReport returns attestation report bytes. +func FetchRawSNPReport(ctx context.Context, reportData []byte) ([]byte, error) { + if pspDriverError != nil { + return nil, pspDriverError + } + + if !pspDriverStarted { + return nil, errors.New("PSP driver is not started") + } + + var reportDataBuf [SNPPSP_API_REPORT_DATA_SIZE]uint8 + + if reportData != nil { + if len(reportData) > SNPPSP_API_REPORT_DATA_SIZE { + return nil, fmt.Errorf("reportData too large: %s", reportData) + } + copy(reportDataBuf[:], reportData) + } + + var report [SNPPSP_API_ATTESTATION_REPORT_SIZE]uint8 + var guestRequestResult SNPPSPGuestRequestResult + + // Fetch attestation report + ret, _, _ := fetchAttestationReportProc.Call( + uintptr(unsafe.Pointer(&reportDataBuf[0])), + uintptr(unsafe.Pointer(&guestRequestResult)), + uintptr(unsafe.Pointer(&report[0]))) + + if ret != SNPPSP_API_STATUS_SUCCESS { + log.G(ctx).Errorf("Failed to fetch attestation report. res: 0x%x, DriverStatus: 0x%x, PspStatus: 0x%x\n", + ret, guestRequestResult.DriverStatus, guestRequestResult.PspStatus) + os.Exit(1) + } + + return report[:], nil +} + +// FetchParsedSNPReport parses raw attestation response into proper structs. +func FetchParsedSNPReport(ctx context.Context, reportData []byte) (Report, error) { + rawBytes, err := FetchRawSNPReport(ctx, reportData) + if err != nil { + return Report{}, err + } + + var r report + buf := bytes.NewBuffer(rawBytes) + if err := binary.Read(buf, binary.LittleEndian, &r); err != nil { + return Report{}, err + } + return r.report(), nil +} + +// TODO: Based on internal\guest\runtime\hcsv2\hostdata.go and it's duplicated. +// ValidateHostData fetches SNP report (if applicable) and validates `hostData` against +// HostData set at UVM launch. +func ValidateHostData(ctx context.Context, hostData []byte) error { + // If the UVM is not SNP, then don't try to fetch an SNP report. + isSnpMode, err := IsSNPMode(ctx) + if err != nil { + return err + } + if !isSnpMode { + return nil + } + report, err := FetchParsedSNPReport(ctx, nil) + if err != nil { + return err + } + + if !bytes.Equal(hostData, report.HostData[:]) { + return fmt.Errorf( + "security policy digest %q doesn't match HostData provided at launch %q", + hostData, + report.HostData[:], + ) + } + + return nil +} From 35d23c8c2325b791edcd86946eddb1ffdf7576f5 Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 15 Sep 2025 15:08:05 +0100 Subject: [PATCH 2/8] Always use deny initialPolicyStance Signed-off-by: Takuro Sato --- cmd/gcs-sidecar/main.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/cmd/gcs-sidecar/main.go b/cmd/gcs-sidecar/main.go index 7e9a3ff865..ecb9cd7faf 100644 --- a/cmd/gcs-sidecar/main.go +++ b/cmd/gcs-sidecar/main.go @@ -230,15 +230,10 @@ func main() { } // gcs-sidecar can be used for non-confidentail hyperv wcow - // as well. So we do not always want to check for initialPolicyStance + // as well. So we do not always want to check for initialPolicyStance. + // While it's used only for confidential cwow, always use "deny" as initial policy. var initialEnforcer securitypolicy.SecurityPolicyEnforcer - initialPolicyStance := "allow" - if pspdriver.GetPspDriverError() != nil || snpMode { - // If the driver failed to start, policy should keep returning "deny" for anything. - // For SNP environment, the initial policy is "deny" but it will be updated - // per the user's security policy. - initialPolicyStance = "deny" - } + initialPolicyStance := "deny" switch initialPolicyStance { case "allow": initialEnforcer = &securitypolicy.OpenDoorSecurityPolicyEnforcer{} From 2b0b7d49110821adc924f8dae512fcec158fc823 Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 15 Sep 2025 16:16:04 +0100 Subject: [PATCH 3/8] Fix golangci-lint errors Signed-off-by: Takuro Sato --- cmd/gcs-sidecar/main.go | 9 +------- internal/gcs-sidecar/host.go | 6 ++--- internal/pspdriver/pspdriver.go | 39 +++++++++++++++++++-------------- 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/cmd/gcs-sidecar/main.go b/cmd/gcs-sidecar/main.go index ecb9cd7faf..199152880e 100644 --- a/cmd/gcs-sidecar/main.go +++ b/cmd/gcs-sidecar/main.go @@ -215,18 +215,11 @@ func main() { return } - var snpMode = false if err := pspdriver.StartPSPDriver(ctx); err != nil { // When error happens, pspdriver.GetPspDriverError() returns true. - // When it happens, gcs-sidecar should keep the initial "deny" policy + // In that case, gcs-sidecar should keep the initial "deny" policy // and reject all requests from the host. logrus.WithError(err).Errorf("failed to start PSP driver") - } else { - snpMode, err = pspdriver.IsSNPMode(ctx) - if err != nil { - logrus.WithError(err).Errorf("failed to check SNP mode: %v", err) - } - logrus.Tracef("SNP mode: %v", snpMode) } // gcs-sidecar can be used for non-confidentail hyperv wcow diff --git a/internal/gcs-sidecar/host.go b/internal/gcs-sidecar/host.go index 03979ee31c..9e0602aaa8 100644 --- a/internal/gcs-sidecar/host.go +++ b/internal/gcs-sidecar/host.go @@ -5,13 +5,13 @@ package bridge import ( "context" - "errors" "fmt" "sync" "github.com/Microsoft/hcsshim/internal/protocol/guestresource" "github.com/Microsoft/hcsshim/internal/pspdriver" "github.com/Microsoft/hcsshim/pkg/securitypolicy" + "github.com/pkg/errors" ) type Host struct { @@ -42,9 +42,9 @@ func (h *Host) SetWCOWConfidentialUVMOptions(ctx context.Context, securityPolicy return errors.New("security policy has already been set") } - if pspdriver.GetPspDriverError() != nil { + if err := pspdriver.GetPspDriverError(); err != nil { // For this case gcs-sidecar will keep initial deny policy. - return fmt.Errorf("occurred error while using PSP driver: %v", pspdriver.GetPspDriverError()) + return errors.Wrapf(err, "an error occurred while using PSP driver") } // Fetch report and validate host_data diff --git a/internal/pspdriver/pspdriver.go b/internal/pspdriver/pspdriver.go index cd2b07836f..764b65d054 100644 --- a/internal/pspdriver/pspdriver.go +++ b/internal/pspdriver/pspdriver.go @@ -25,20 +25,20 @@ const ( ) const ( - SNPPSP_API_STATUS_SUCCESS = 0x00000000 - SNPPSP_API_STATUS_UNSUCCESSFUL = 0x00000001 - SNPPSP_API_STATUS_DRIVER_UNSUCCESSFUL = 0x00000003 - SNPPSP_API_STATUS_PSP_UNSUCCESSFUL = 0x00000004 - SNPPSP_API_STATUS_INVALID_PARAMETER = 0x00000005 - SNPPSP_API_STATUS_DEVICE_NOT_AVAILABLE = 0x00000006 + SnpPspAPIStatusSuccess = 0x00000000 + SnpPspAPIStatusUnsuccessful = 0x00000001 + SnpPspAPIStatusDriverUnsuccessful = 0x00000003 + SnpPspAPIStatusPspUnsuccessful = 0x00000004 + SnpPspAPIStatusInvalidParameter = 0x00000005 + SnpPspAPIStatusDeviceNotAvailable = 0x00000006 ) // TODO: Fix duplication with pkg/amdsevsnp and merge this into it. const ( - SNPPSP_API_REPORT_DATA_SIZE = 64 - SNPPSP_API_REPORT_HOST_DATA_SIZE = 32 - SNPPSP_API_ATTESTATION_REPORT_SIZE = 0x4A0 + SnpPspReportDataSize = 64 + SnpPspReportHostDataSize = 32 + SnpPspAttestationReportSize = 0x4A0 ) type SNPPSPGuestRequestResult struct { @@ -58,9 +58,9 @@ type report struct { PlatformInfo uint64 AuthorKeyEn uint32 Reserved1 uint32 - ReportData [SNPPSP_API_REPORT_DATA_SIZE]byte + ReportData [SnpPspReportDataSize]byte Measurement [48]byte - HostData [SNPPSP_API_REPORT_HOST_DATA_SIZE]byte + HostData [SnpPspReportHostDataSize]byte IDKeyDigest [48]byte AuthorKeyDigest [48]byte ReportID [32]byte @@ -160,7 +160,12 @@ func StartPSPDriver(ctx context.Context) error { if err != nil { return errors.Wrap(err, "Failed to connect to service manager") } - defer m.Disconnect() + defer func() { + if derr := m.Disconnect(); derr != nil { + // Log the error on disconnect but do not override the returned error. + log.G(ctx).Warnf("Failed to disconnect from service manager: %v", derr) + } + }() // Open the service s, err := m.OpenService(serviceName) @@ -226,7 +231,7 @@ func IsSNPMode(ctx context.Context) (bool, error) { // snpMode is defined as BOOLEAN (= byte) var snpMode uint8 ret, _, _ := isSnpModeProc.Call(uintptr(unsafe.Pointer(&snpMode))) - if ret != SNPPSP_API_STATUS_SUCCESS { + if ret != SnpPspAPIStatusSuccess { pspDriverError = errors.Errorf("failed to determine if it's in SNP VM. SNPPSP_API_STATUS: 0x%x", ret) return false, pspDriverError } @@ -244,16 +249,16 @@ func FetchRawSNPReport(ctx context.Context, reportData []byte) ([]byte, error) { return nil, errors.New("PSP driver is not started") } - var reportDataBuf [SNPPSP_API_REPORT_DATA_SIZE]uint8 + var reportDataBuf [SnpPspReportDataSize]uint8 if reportData != nil { - if len(reportData) > SNPPSP_API_REPORT_DATA_SIZE { + if len(reportData) > SnpPspReportDataSize { return nil, fmt.Errorf("reportData too large: %s", reportData) } copy(reportDataBuf[:], reportData) } - var report [SNPPSP_API_ATTESTATION_REPORT_SIZE]uint8 + var report [SnpPspAttestationReportSize]uint8 var guestRequestResult SNPPSPGuestRequestResult // Fetch attestation report @@ -262,7 +267,7 @@ func FetchRawSNPReport(ctx context.Context, reportData []byte) ([]byte, error) { uintptr(unsafe.Pointer(&guestRequestResult)), uintptr(unsafe.Pointer(&report[0]))) - if ret != SNPPSP_API_STATUS_SUCCESS { + if ret != SnpPspAPIStatusSuccess { log.G(ctx).Errorf("Failed to fetch attestation report. res: 0x%x, DriverStatus: 0x%x, PspStatus: 0x%x\n", ret, guestRequestResult.DriverStatus, guestRequestResult.PspStatus) os.Exit(1) From 2add9fc6e31713f9811c20b002921bc850b0ebf5 Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Mon, 15 Sep 2025 16:32:49 +0100 Subject: [PATCH 4/8] Remove unnecessary comment Signed-off-by: Takuro Sato --- internal/pspdriver/pspdriver.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/pspdriver/pspdriver.go b/internal/pspdriver/pspdriver.go index 764b65d054..ec8af8f926 100644 --- a/internal/pspdriver/pspdriver.go +++ b/internal/pspdriver/pspdriver.go @@ -152,8 +152,6 @@ var ( pspDriverError error = nil ) -// TODO: Should we use ctx for functions here? - func StartPSPDriver(ctx context.Context) error { // Connect to the Service Control Manager m, err := mgr.Connect() From 43d5d74b53c0b988dd482904ef4dfe33dbd9a7c0 Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Tue, 16 Sep 2025 13:56:11 +0100 Subject: [PATCH 5/8] Remove redundasnt switch statement Signed-off-by: Takuro Sato --- cmd/gcs-sidecar/main.go | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/cmd/gcs-sidecar/main.go b/cmd/gcs-sidecar/main.go index 199152880e..f69fad0c54 100644 --- a/cmd/gcs-sidecar/main.go +++ b/cmd/gcs-sidecar/main.go @@ -225,18 +225,7 @@ func main() { // gcs-sidecar can be used for non-confidentail hyperv wcow // as well. So we do not always want to check for initialPolicyStance. // While it's used only for confidential cwow, always use "deny" as initial policy. - var initialEnforcer securitypolicy.SecurityPolicyEnforcer - initialPolicyStance := "deny" - switch initialPolicyStance { - case "allow": - initialEnforcer = &securitypolicy.OpenDoorSecurityPolicyEnforcer{} - logrus.Tracef("initial-policy-stance: allow") - case "deny": - initialEnforcer = &securitypolicy.ClosedDoorSecurityPolicyEnforcer{} - logrus.Tracef("initial-policy-stance: deny") - default: - logrus.Error("unknown initial-policy-stance") - } + initialEnforcer := &securitypolicy.ClosedDoorSecurityPolicyEnforcer{} // 3. Create bridge and initializa brdg := sidecar.NewBridge(shimCon, gcsCon, initialEnforcer) From 09a5eaf6120424920dc7554d0a736ab3ba8adb7f Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Tue, 16 Sep 2025 13:58:50 +0100 Subject: [PATCH 6/8] Fix status code values + Add comments Signed-off-by: Takuro Sato --- internal/pspdriver/pspdriver.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/internal/pspdriver/pspdriver.go b/internal/pspdriver/pspdriver.go index ec8af8f926..9b85dcdfc1 100644 --- a/internal/pspdriver/pspdriver.go +++ b/internal/pspdriver/pspdriver.go @@ -27,10 +27,10 @@ const ( const ( SnpPspAPIStatusSuccess = 0x00000000 SnpPspAPIStatusUnsuccessful = 0x00000001 - SnpPspAPIStatusDriverUnsuccessful = 0x00000003 - SnpPspAPIStatusPspUnsuccessful = 0x00000004 - SnpPspAPIStatusInvalidParameter = 0x00000005 - SnpPspAPIStatusDeviceNotAvailable = 0x00000006 + SnpPspAPIStatusDriverUnsuccessful = 0x00000002 + SnpPspAPIStatusPspUnsuccessful = 0x00000003 + SnpPspAPIStatusInvalidParameter = 0x00000004 + SnpPspAPIStatusDeviceNotAvailable = 0x00000005 ) // TODO: Fix duplication with pkg/amdsevsnp and merge this into it. @@ -46,6 +46,9 @@ type SNPPSPGuestRequestResult struct { PspStatus uint64 } +// Type used by FetchParsedSNPReport. +// This it converted to the public type `Report` +// by `func (sr *report) report() Report`. type report struct { Version uint32 GuestSVN uint32 @@ -76,6 +79,10 @@ type report struct { } // Report represents parsed attestation report. +// Fields with string type is hex-encoded values of the corresponding byte arrays. +// Based on Table 23 of 'SEV-ES Guest-Hypervisor Communication Block Standardization'. +// +// https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56421.pdf type Report struct { Version uint32 GuestSVN uint32 From 2f21484a4dd2ff4fe500c6eb9d169a8efc854bb4 Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Tue, 16 Sep 2025 16:20:01 +0100 Subject: [PATCH 7/8] Use mkwinsyscall for amdsnppspapi.dll Signed-off-by: Takuro Sato --- internal/pspdriver/pspdriver.go | 55 +++++++++++++++++------------ internal/winapi/amdsnp.go | 11 ++++++ internal/winapi/zsyscall_windows.go | 47 +++++++++++++++++++----- 3 files changed, 81 insertions(+), 32 deletions(-) create mode 100644 internal/winapi/amdsnp.go diff --git a/internal/pspdriver/pspdriver.go b/internal/pspdriver/pspdriver.go index 9b85dcdfc1..db41384853 100644 --- a/internal/pspdriver/pspdriver.go +++ b/internal/pspdriver/pspdriver.go @@ -9,13 +9,11 @@ import ( "encoding/binary" "encoding/hex" "fmt" - "os" "time" - "unsafe" "github.com/Microsoft/hcsshim/internal/log" + "github.com/Microsoft/hcsshim/internal/winapi" "github.com/pkg/errors" - "golang.org/x/sys/windows" "golang.org/x/sys/windows/svc" "golang.org/x/sys/windows/svc/mgr" ) @@ -148,11 +146,7 @@ func mirrorBytes(b []byte) []byte { } var ( - amdsnppspapi = windows.NewLazySystemDLL("amdsnppspapi.dll") - // It will panic if the function is not found when .Call() is called. - isSnpModeProc = amdsnppspapi.NewProc("SnpPspIsSnpMode") - fetchAttestationReportProc = amdsnppspapi.NewProc("SnpPspFetchAttestationReport") - pspDriverStarted = false + pspDriverStarted = false // The error needs to be stored to be retrieved later. // When driver or its dll fails, gcs-sidecar doesn't // set security policy and keep the initial deny policy. @@ -235,9 +229,19 @@ func IsSNPMode(ctx context.Context) (bool, error) { // snpMode is defined as BOOLEAN (= byte) var snpMode uint8 - ret, _, _ := isSnpModeProc.Call(uintptr(unsafe.Pointer(&snpMode))) - if ret != SnpPspAPIStatusSuccess { - pspDriverError = errors.Errorf("failed to determine if it's in SNP VM. SNPPSP_API_STATUS: 0x%x", ret) + ret, err := winapi.SnpPspIsSnpMode(&snpMode) + + if ret != SnpPspAPIStatusSuccess || err != nil { + errMessage := "" + if err != nil { + // err is not nil either when `winapi` didn't find the API or when ret is not success. + // In case of the former, ret is meaningless because ret is returned by the dll. + // In case of the latter, we don't need to print err. + // We can't tell which case it is here, we print all the information we have. + // We could avoid this by loading the dll in this package, but we use `winapi` for consistency with existing code. + errMessage = fmt.Sprintf(", err: %v", err) + } + pspDriverError = errors.Errorf("failed to determine if it's in SNP VM. SNPPSP_API_STATUS: 0x%x%s", ret, errMessage) return false, pspDriverError } @@ -264,18 +268,23 @@ func FetchRawSNPReport(ctx context.Context, reportData []byte) ([]byte, error) { } var report [SnpPspAttestationReportSize]uint8 - var guestRequestResult SNPPSPGuestRequestResult - - // Fetch attestation report - ret, _, _ := fetchAttestationReportProc.Call( - uintptr(unsafe.Pointer(&reportDataBuf[0])), - uintptr(unsafe.Pointer(&guestRequestResult)), - uintptr(unsafe.Pointer(&report[0]))) - - if ret != SnpPspAPIStatusSuccess { - log.G(ctx).Errorf("Failed to fetch attestation report. res: 0x%x, DriverStatus: 0x%x, PspStatus: 0x%x\n", - ret, guestRequestResult.DriverStatus, guestRequestResult.PspStatus) - os.Exit(1) + var guestRequestResult winapi.SNPPSPGuestRequestResult + + // Fetch attestation report using generated winapi wrapper + ret, err := winapi.SnpPspFetchAttestationReport(&reportDataBuf[0], &guestRequestResult, &report[0]) + if ret != SnpPspAPIStatusSuccess || err != nil { + errMessage := "" + if err != nil { + // err is not nil either when `winapi` didn't find the API or when ret is not success. + // In case of the former, ret and guestRequestResult are meaningless because they are returned by the dll. + // In case of the latter, we don't need to print err. + // We can't tell which case it is here, we print all the information we have. + // We could avoid this by loading the dll in this package, but we use `winapi` for consistency with existing code. + errMessage = fmt.Sprintf(", err: %v", err) + } + pspDriverError = errors.Errorf("failed to fetch attestation report. res: 0x%x, DriverStatus: 0x%x, PspStatus: 0x%x%s", + ret, guestRequestResult.DriverStatus, guestRequestResult.PspStatus, errMessage) + return nil, pspDriverError } return report[:], nil diff --git a/internal/winapi/amdsnp.go b/internal/winapi/amdsnp.go new file mode 100644 index 0000000000..5e0f9cd2c5 --- /dev/null +++ b/internal/winapi/amdsnp.go @@ -0,0 +1,11 @@ +//go:build windows + +package winapi + +type SNPPSPGuestRequestResult struct { + DriverStatus uint32 + PspStatus uint64 +} + +//sys SnpPspIsSnpMode(snpMode *uint8) (ret uint32, err error) [failretval>0] = amdsnppspapi.SnpPspIsSnpMode? +//sys SnpPspFetchAttestationReport(reportData *uint8, guestRequestResult *SNPPSPGuestRequestResult, report *uint8) (ret uint32, err error) [failretval>0] = amdsnppspapi.SnpPspFetchAttestationReport? diff --git a/internal/winapi/zsyscall_windows.go b/internal/winapi/zsyscall_windows.go index a7eea44ec7..761e09bbd4 100644 --- a/internal/winapi/zsyscall_windows.go +++ b/internal/winapi/zsyscall_windows.go @@ -37,17 +37,20 @@ func errnoErr(e syscall.Errno) error { } var ( - modadvapi32 = windows.NewLazySystemDLL("advapi32.dll") - modbindfltapi = windows.NewLazySystemDLL("bindfltapi.dll") - modcfgmgr32 = windows.NewLazySystemDLL("cfgmgr32.dll") - modcimfs = windows.NewLazySystemDLL("cimfs.dll") - modiphlpapi = windows.NewLazySystemDLL("iphlpapi.dll") - modkernel32 = windows.NewLazySystemDLL("kernel32.dll") - modnetapi32 = windows.NewLazySystemDLL("netapi32.dll") - modntdll = windows.NewLazySystemDLL("ntdll.dll") - modoffreg = windows.NewLazySystemDLL("offreg.dll") + modadvapi32 = windows.NewLazySystemDLL("advapi32.dll") + modamdsnppspapi = windows.NewLazySystemDLL("amdsnppspapi.dll") + modbindfltapi = windows.NewLazySystemDLL("bindfltapi.dll") + modcfgmgr32 = windows.NewLazySystemDLL("cfgmgr32.dll") + modcimfs = windows.NewLazySystemDLL("cimfs.dll") + modiphlpapi = windows.NewLazySystemDLL("iphlpapi.dll") + modkernel32 = windows.NewLazySystemDLL("kernel32.dll") + modnetapi32 = windows.NewLazySystemDLL("netapi32.dll") + modntdll = windows.NewLazySystemDLL("ntdll.dll") + modoffreg = windows.NewLazySystemDLL("offreg.dll") procLogonUserW = modadvapi32.NewProc("LogonUserW") + procSnpPspFetchAttestationReport = modamdsnppspapi.NewProc("SnpPspFetchAttestationReport") + procSnpPspIsSnpMode = modamdsnppspapi.NewProc("SnpPspIsSnpMode") procBfSetupFilter = modbindfltapi.NewProc("BfSetupFilter") procCM_Get_DevNode_PropertyW = modcfgmgr32.NewProc("CM_Get_DevNode_PropertyW") procCM_Get_Device_ID_ListA = modcfgmgr32.NewProc("CM_Get_Device_ID_ListA") @@ -124,6 +127,32 @@ func LogonUser(username *uint16, domain *uint16, password *uint16, logonType uin return } +func SnpPspFetchAttestationReport(reportData *uint8, guestRequestResult *SNPPSPGuestRequestResult, report *uint8) (ret uint32, err error) { + err = procSnpPspFetchAttestationReport.Find() + if err != nil { + return + } + r0, _, e1 := syscall.SyscallN(procSnpPspFetchAttestationReport.Addr(), uintptr(unsafe.Pointer(reportData)), uintptr(unsafe.Pointer(guestRequestResult)), uintptr(unsafe.Pointer(report))) + ret = uint32(r0) + if ret > 0 { + err = errnoErr(e1) + } + return +} + +func SnpPspIsSnpMode(snpMode *uint8) (ret uint32, err error) { + err = procSnpPspIsSnpMode.Find() + if err != nil { + return + } + r0, _, e1 := syscall.SyscallN(procSnpPspIsSnpMode.Addr(), uintptr(unsafe.Pointer(snpMode))) + ret = uint32(r0) + if ret > 0 { + err = errnoErr(e1) + } + return +} + func BfSetupFilter(jobHandle windows.Handle, flags uint32, virtRootPath *uint16, virtTargetPath *uint16, virtExceptions **uint16, virtExceptionPathCount uint32) (hr error) { hr = procBfSetupFilter.Find() if hr != nil { From affe22609d1c92ea828daa954a3c961b5786930d Mon Sep 17 00:00:00 2001 From: Takuro Sato Date: Wed, 17 Sep 2025 10:07:09 +0100 Subject: [PATCH 8/8] Update comment Signed-off-by: Takuro Sato --- cmd/gcs-sidecar/main.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/gcs-sidecar/main.go b/cmd/gcs-sidecar/main.go index f69fad0c54..71e27dbc05 100644 --- a/cmd/gcs-sidecar/main.go +++ b/cmd/gcs-sidecar/main.go @@ -222,9 +222,8 @@ func main() { logrus.WithError(err).Errorf("failed to start PSP driver") } - // gcs-sidecar can be used for non-confidentail hyperv wcow - // as well. So we do not always want to check for initialPolicyStance. - // While it's used only for confidential cwow, always use "deny" as initial policy. + // Use "deny" policy as initial enforcer. + // This is updated later with user provided policy. initialEnforcer := &securitypolicy.ClosedDoorSecurityPolicyEnforcer{} // 3. Create bridge and initializa