Attach EFI VHD in read-only mode by default

EFI VHDs should always be attached as read-only by default to block UVMs from writing to
it and corrupting its contents. A new annotation is added to allow attaching EFI VHDs in
writable mode when debugging boot failures and such. When this annotation is included a
copy of the EFI VHD is made next to the scratch VHD. This is based on the assumption that
generally the scratch of the UVM would be stored in its own snapshot directory so adding
another VHD in there shouldn't be a problem. It should get cleaned up when the snapshot is
removed.

This commit also adds the code to always grant VM group access to the VHDs and guest state
files to avoid access denied failures.

Signed-off-by: Amit Barve <ambarve@microsoft.com>

Author: ambarve

cmd/containerd-shim-runhcs-v1/pod.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/Microsoft/hcsshim/internal/copyfile"
 	"github.com/Microsoft/hcsshim/internal/layers"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/oci"
@@ -58,6 +59,21 @@ func initializeWCOWBootFiles(ctx context.Context, wopts *uvm.OptionsWCOW, rootfs
 		// we can enable this.
 		return fmt.Errorf("hyperv isolation is not supported with block CIM layers yet")
 	}
+
+	// writable EFI VHD is a valid config for both confidential and regular hyperv
+	// isolated WCOW. Override the default value here if required.
+	if wopts.WritableEFI {
+		// Make a copy of EFI VHD, we can't risk the UVM accidentally modifying
+		// the original VHD.  make copy next to the scratch VHD, this assumes that
+		// the scratch is located in the separate directory dedicated for this
+		// UVM.
+		writableEFIVHDPath := filepath.Join(filepath.Dir(wopts.BootFiles.BlockCIMFiles.ScratchVHDPath), filepath.Base(wopts.BootFiles.BlockCIMFiles.EFIVHDPath))
+		if err := copyfile.CopyFile(ctx, wopts.BootFiles.BlockCIMFiles.EFIVHDPath, writableEFIVHDPath, false); err != nil {
+			return fmt.Errorf("failed to copy EFI VHD at %s: %w", writableEFIVHDPath, err)
+		}
+		wopts.BootFiles.BlockCIMFiles.EFIVHDPath = writableEFIVHDPath
+	}
+
 	return nil
 }
 

internal/oci/uvm.go

@@ -376,6 +376,9 @@ func SpecToUVMCreateOpts(ctx context.Context, s *specs.Spec, id, owner string) (
 		wopts.AdditionalRegistryKeys = append(wopts.AdditionalRegistryKeys, parseAdditionalRegistryValues(ctx, s.Annotations)...)
 		handleAnnotationFullyPhysicallyBacked(ctx, s.Annotations, wopts)
 
+		// Writable EFI is valid for both confidential and regular Hyper-V isolated WCOW.
+		wopts.WritableEFI = ParseAnnotationsBool(ctx, s.Annotations, annotations.WCOWWritableEFI, wopts.WritableEFI)
+
 		// Handle WCOW security policy settings
 		if err := handleWCOWSecurityPolicy(ctx, s.Annotations, wopts); err != nil {
 			return nil, err

internal/oci/uvm_test.go

@@ -139,6 +139,10 @@ func Test_SpecToUVMCreateOptions_WCOW_Confidential_Defaults(t *testing.T) {
 	if !wopts.SecurityPolicyEnabled {
 		t.Fatal("SecurityPolicyEnabled should be true when WCOWSecurityPolicy is set")
 	}
+	// Writable EFI should default to false unless explicitly enabled
+	if wopts.WritableEFI {
+		t.Fatal("WritableEFI should default to false when not specified")
+	}
 	if wopts.MemorySizeInMB != 2048 {
 		t.Fatalf("expected MemorySizeInMB to default to 2048, got %d", wopts.MemorySizeInMB)
 	}
@@ -159,6 +163,47 @@ func Test_SpecToUVMCreateOptions_WCOW_Confidential_Defaults(t *testing.T) {
 	}
 }
 
+func Test_SpecToUVMCreateOptions_WCOW_Confidential_WritableEFI_Enabled(t *testing.T) {
+	s := &specs.Spec{
+		Windows: &specs.Windows{HyperV: &specs.WindowsHyperV{}},
+		Annotations: map[string]string{
+			annotations.WCOWSecurityPolicy: "test-policy",
+			annotations.WCOWWritableEFI:    "true",
+		},
+	}
+
+	opts, err := SpecToUVMCreateOpts(context.Background(), s, t.Name(), "")
+	if err != nil {
+		t.Fatalf("unexpected error generating UVM opts: %v", err)
+	}
+
+	wopts := (opts).(*uvm.OptionsWCOW)
+	if !wopts.WritableEFI {
+		t.Fatal("WritableEFI should be true when WCOWWritableEFI annotation is set to true")
+	}
+}
+
+func Test_SpecToUVMCreateOptions_WCOW_NonConfidential_WritableEFI_Enabled(t *testing.T) {
+	s := &specs.Spec{
+		Windows: &specs.Windows{HyperV: &specs.WindowsHyperV{}},
+		Annotations: map[string]string{
+			// No WCOWSecurityPolicy means non-confidential path
+			annotations.WCOWWritableEFI: "true",
+		},
+	}
+
+	opts, err := SpecToUVMCreateOpts(context.Background(), s, t.Name(), "")
+	if err != nil {
+		t.Fatalf("unexpected error generating UVM opts: %v", err)
+	}
+
+	wopts := (opts).(*uvm.OptionsWCOW)
+	// WritableEFI should be respected for non-confidential as well
+	if !wopts.WritableEFI {
+		t.Fatal("WritableEFI should be true for non-confidential when WCOWWritableEFI is set")
+	}
+}
+
 func Test_SpecToUVMCreateOptions_WCOW_Confidential_ErrorOnLowMemory(t *testing.T) {
 	s := &specs.Spec{
 		Windows: &specs.Windows{HyperV: &specs.WindowsHyperV{}},
@@ -191,13 +236,13 @@ func Test_SpecToUVMCreateOptions_WCOW_Confidential_Overrides(t *testing.T) {
 	s := &specs.Spec{
 		Windows: &specs.Windows{HyperV: &specs.WindowsHyperV{}},
 		Annotations: map[string]string{
-			annotations.WCOWSecurityPolicy:        "test-policy",
-			annotations.MemorySizeInMB:            "4096",
-			annotations.AllowOvercommit:           "false",
+			annotations.WCOWSecurityPolicy:         "test-policy",
+			annotations.MemorySizeInMB:             "4096",
+			annotations.AllowOvercommit:            "false",
 			annotations.WCOWSecurityPolicyEnforcer: "rego",
-			annotations.WCOWDisableSecureBoot:     "true",
-			annotations.WCOWGuestStateFile:        "C:\\custom\\cwcow.vmgs",
-			annotations.WCOWIsolationType:         "VirtualizationBasedSecurity",
+			annotations.WCOWDisableSecureBoot:      "true",
+			annotations.WCOWGuestStateFile:         "C:\\custom\\cwcow.vmgs",
+			annotations.WCOWIsolationType:          "VirtualizationBasedSecurity",
 		},
 	}
 

internal/uvm/create_wcow.go

@@ -41,6 +41,7 @@ type ConfidentialWCOWOptions struct {
 	IsolationType      string
 	DisableSecureBoot  bool
 	FirmwareParameters string
+	WritableEFI        bool
 }
 
 // OptionsWCOW are the set of options passed to CreateWCOW() to create a utility vm.
@@ -384,26 +385,37 @@ func prepareSecurityConfigDoc(ctx context.Context, uvm *UtilityVM, opts *Options
 		Minor: 0,
 	}
 
+	// TODO(ambarve): only scratch VHD is unique per VM, EFI & Boot CIM VHDs are
+	// shared across UVMs, so we don't need to assign VM group access to them every
+	// time. It should have been done once while deploying the package.
+	if err := wclayer.GrantVmAccess(ctx, uvm.id, opts.BootFiles.BlockCIMFiles.EFIVHDPath); err != nil {
+		return nil, errors.Wrap(err, "failed to grant vm access to EFI VHD")
+	}
+
 	if err := wclayer.GrantVmAccess(ctx, uvm.id, opts.BootFiles.BlockCIMFiles.BootCIMVHDPath); err != nil {
-		return nil, errors.Wrap(err, "failed to grant vm access to boot CIM VHD")
+		return nil, errors.Wrap(err, "failed to grant vm access to Boot CIM VHD")
 	}
 
-	if err := wclayer.GrantVmAccess(ctx, uvm.id, opts.BootFiles.BlockCIMFiles.EFIVHDPath); err != nil {
-		return nil, errors.Wrap(err, "failed to grant vm access to EFI VHD")
+	if err := wclayer.GrantVmAccess(ctx, uvm.id, opts.GuestStateFilePath); err != nil {
+		return nil, errors.Wrap(err, "failed to grant vm access to guest state file")
 	}
 
 	if err := wclayer.GrantVmAccess(ctx, uvm.id, opts.BootFiles.BlockCIMFiles.ScratchVHDPath); err != nil {
 		return nil, errors.Wrap(err, "failed to grant vm access to scratch VHD")
 	}
 
+	// boot depends on scratch being attached at LUN 0, it MUST ALWAYS remain at LUN 0
 	doc.VirtualMachine.Devices.Scsi[guestrequest.ScsiControllerGuids[0]].Attachments["0"] = hcsschema.Attachment{
 		Path:  opts.BootFiles.BlockCIMFiles.ScratchVHDPath,
 		Type_: "VirtualDisk",
 	}
+
 	doc.VirtualMachine.Devices.Scsi[guestrequest.ScsiControllerGuids[0]].Attachments["1"] = hcsschema.Attachment{
-		Path:  opts.BootFiles.BlockCIMFiles.EFIVHDPath,
-		Type_: "VirtualDisk",
+		Path:     opts.BootFiles.BlockCIMFiles.EFIVHDPath,
+		Type_:    "VirtualDisk",
+		ReadOnly: !opts.WritableEFI,
 	}
+
 	doc.VirtualMachine.Devices.Scsi[guestrequest.ScsiControllerGuids[0]].Attachments["2"] = hcsschema.Attachment{
 		Path:     opts.BootFiles.BlockCIMFiles.BootCIMVHDPath,
 		Type_:    "VirtualDisk",

pkg/annotations/annotations.go

@@ -205,6 +205,10 @@ const (
 	// Allows disabling secure boot for testing and debugging scenarios, secure boot doesn't apply to confidential LCOW so
 	// this is a WCOW only config
 	WCOWDisableSecureBoot = "io.microsoft.virtualmachine.wcow.no_secure_boot"
+
+	// Attaches the EFI/boot VHD in the writable mode (instead of the default read-only mode). This is usually required
+	// when debugging boot to capture bootstat traces.
+	WCOWWritableEFI = "io.microsoft.virtualmachine.wcow.writable_efi"
 )
 
 // WCOW host process container annotations.