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 {
+		// when attaching EFI in writable mode, make a copy, we don't want the UVM
+		// to modify the main copy.  We currently 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

@@ -245,6 +245,7 @@ func handleWCOWSecurityPolicy(ctx context.Context, a map[string]string, wopts *u
 	wopts.SecurityPolicyEnforcer = ParseAnnotationsString(a, annotations.WCOWSecurityPolicyEnforcer, wopts.SecurityPolicyEnforcer)
 	wopts.DisableSecureBoot = ParseAnnotationsBool(ctx, a, annotations.WCOWDisableSecureBoot, false)
 	wopts.GuestStateFilePath = ParseAnnotationsString(a, annotations.WCOWGuestStateFile, uvm.GetDefaultConfidentialVMGSPath())
+	wopts.WritableEFI = ParseAnnotationsBool(ctx, a, annotations.WCOWWritableEFI, false)
 	if ParseAnnotationsBool(ctx, a, annotations.WCOWNoSecurityHardware, false) {
 		wopts.NoSecurityHardware = true
 		wopts.IsolationType = "VirtualizationBasedSecurity"

internal/uvm/create_wcow.go

@@ -42,6 +42,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.
@@ -385,26 +386,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.