From 24e511c6d6cc8b074e89dea4bc18a8220adf89ad Mon Sep 17 00:00:00 2001 From: Bugs5382 <12115015+Bugs5382@users.noreply.github.com> Date: Wed, 14 Oct 2026 11:15:20 -0400 Subject: [PATCH] fix(imageupgrade): serialise upgrade calls and sync the ESP directory StageImage, RollbackImage and ActivateImage took no lock, so two upgrade calls at the same time could share BOOTX64.NEW.EFI and leave a half-written boot image. A shared mutex on the node's image upgrader now refuses a call made while another of the three is already running with FailedPrecondition, rather than queuing it or racing it. The rename that swaps the active image in is also not durable on its own: vfat needs the directory entry fsynced, which a file's own fsync does not cover. Stage and Rollback now fsync the ESP's mounted directory right after that rename, so a power loss right after leaves the old or the new image, never neither. Signed-off-by: Bugs5382 <12115015+Bugs5382@users.noreply.github.com> --- internal/grpc/imageupgrade.go | 19 ++++ internal/imageupgrade/imageupgrade.go | 13 +++ internal/imageupgrade/imageupgrade_test.go | 91 +++++++++++++++++- internal/init/imageupgrade.go | 40 ++++++++ internal/init/imageupgrade_test.go | 107 +++++++++++++++++++++ 5 files changed, 269 insertions(+), 1 deletion(-) diff --git a/internal/grpc/imageupgrade.go b/internal/grpc/imageupgrade.go index 3a8f5df..a263c27 100644 --- a/internal/grpc/imageupgrade.go +++ b/internal/grpc/imageupgrade.go @@ -74,6 +74,13 @@ var ErrImageNotResealable = errors.New("grpc: the state key cannot be resealed f // package need not import the implementation. var ErrNoPreviousImage = errors.New("grpc: no previous image to roll back to") +// ErrUpgradeInProgress is returned by an ImageUpgrader's Stage, Rollback or +// Activate when another one of those three calls is already running on this +// node. The handler maps it to FailedPrecondition: the request itself is +// fine, but a second call in flight at the same time as a Stage could share +// the write slot (BOOTX64.NEW.EFI) and leave a half-written boot image. +var ErrUpgradeInProgress = errors.New("grpc: an image upgrade call is already in progress on this node") + // ImageUpgrader replaces the node's CryptOS image without touching its // identity. It is wired on the mTLS and local servers of a running node; the // maintenance servers leave it nil so the image RPCs return Unimplemented @@ -123,6 +130,10 @@ func (s *Server) StageImage(stream grpc.ClientStreamingServer[nodev1.StageImageR return status.Errorf(codes.FailedPrecondition, "StageImage: nothing was written: %v", err) } + if errors.Is(err, ErrUpgradeInProgress) { + return status.Error(codes.FailedPrecondition, + "StageImage: another image upgrade call is already in progress on this node") + } return status.Errorf(codes.Internal, "StageImage: %v", err) } @@ -209,6 +220,10 @@ func (s *Server) RollbackImage(ctx context.Context, _ *nodev1.RollbackImageReque if errors.Is(err, ErrNoPreviousImage) { return nil, status.Error(codes.FailedPrecondition, "RollbackImage: this node has no previous image retained") } + if errors.Is(err, ErrUpgradeInProgress) { + return nil, status.Error(codes.FailedPrecondition, + "RollbackImage: another image upgrade call is already in progress on this node") + } return nil, status.Errorf(codes.Internal, "RollbackImage: %v", err) } @@ -243,6 +258,10 @@ func (s *Server) ActivateImage(ctx context.Context, req *nodev1.ActivateImageReq if errors.Is(err, reset.ErrConfirmMismatch) { return nil, status.Error(codes.PermissionDenied, "ActivateImage: confirmation CN does not match the CA CN") } + if errors.Is(err, ErrUpgradeInProgress) { + return nil, status.Error(codes.FailedPrecondition, + "ActivateImage: another image upgrade call is already in progress on this node") + } return nil, status.Errorf(codes.Internal, "ActivateImage: %v", err) } diff --git a/internal/imageupgrade/imageupgrade.go b/internal/imageupgrade/imageupgrade.go index d38706b..044dbcc 100644 --- a/internal/imageupgrade/imageupgrade.go +++ b/internal/imageupgrade/imageupgrade.go @@ -97,6 +97,13 @@ type FS interface { Remove(rel string) error Rename(from, to string) error WriteFile(rel string, data []byte) error + // Sync fsyncs the ESP's mounted directory. A vfat rename is only a + // directory-entry update; WriteFile's own fsync covers a file's data, not + // that entry. Without this, a power loss right after the rename that swaps + // the active image can leave the directory entry unresolved -- neither + // surely the old image nor surely the new one -- rather than one of the + // two, which is what every caller of Stage and Rollback relies on. + Sync() error } // Stager installs verified images into the ESP slots. @@ -198,6 +205,9 @@ func (s *Stager) Stage(image, signature []byte) (Status, error) { if err := s.fs.Rename(incomingRelPath, ActiveRelPath); err != nil { return Status{}, fmt.Errorf("imageupgrade: activate the incoming image: %w", err) } + if err := s.fs.Sync(); err != nil { + return Status{}, fmt.Errorf("imageupgrade: sync the ESP directory: %w", err) + } return s.Status() } @@ -223,6 +233,9 @@ func (s *Stager) Rollback() error { if err := s.fs.Rename(incomingRelPath, ActiveRelPath); err != nil { return fmt.Errorf("imageupgrade: restore the previous image: %w", err) } + if err := s.fs.Sync(); err != nil { + return fmt.Errorf("imageupgrade: sync the ESP directory: %w", err) + } return nil } diff --git a/internal/imageupgrade/imageupgrade_test.go b/internal/imageupgrade/imageupgrade_test.go index 390644a..7c43a09 100644 --- a/internal/imageupgrade/imageupgrade_test.go +++ b/internal/imageupgrade/imageupgrade_test.go @@ -30,11 +30,15 @@ import ( ) // memFS is the ESP in memory. failWrite lets a test interrupt the sequence at -// the point that decides whether a node is left bootable. +// the point that decides whether a node is left bootable. ops records every +// call in order (including Sync), so a test can assert the directory sync +// happens after the rename that it is meant to make durable. type memFS struct { files map[string][]byte failWrite map[string]error + failSync error writes []string + ops []string } func newMemFS() *memFS { @@ -55,6 +59,7 @@ func (m *memFS) ReadFile(rel string) ([]byte, error) { func (m *memFS) Remove(rel string) error { delete(m.files, rel); return nil } func (m *memFS) Rename(from, to string) error { + m.ops = append(m.ops, "rename:"+from+"->"+to) data, ok := m.files[from] if !ok { return errors.New("not found: " + from) @@ -67,6 +72,7 @@ func (m *memFS) Rename(from, to string) error { func (m *memFS) WriteFile(rel string, data []byte) error { m.writes = append(m.writes, rel) + m.ops = append(m.ops, "write:"+rel) if err := m.failWrite[rel]; err != nil { return err } @@ -75,6 +81,14 @@ func (m *memFS) WriteFile(rel string, data []byte) error { return nil } +// Sync stands in for fsyncing the ESP's mounted directory: the durability +// barrier a vfat rename needs that a file's own fsync does not provide. +func (m *memFS) Sync() error { + m.ops = append(m.ops, "sync") + + return m.failSync +} + type release struct { cert *x509.Certificate key *rsa.PrivateKey @@ -249,6 +263,40 @@ func TestStage_OnANodeWithNoActiveImage(t *testing.T) { } } +// A vfat rename isn't durable across a power loss on its own: the directory +// entry update needs its own fsync. Stage must call it right after the +// rename that makes the incoming image the active one (#373). +func TestStage_SyncsTheESPDirectoryAfterTheRename(t *testing.T) { + rel := newRelease(t) + fs := newMemFS() + fs.files[ActiveRelPath] = []byte("old image") + s, _ := New(fs, rel.cert) + + image := []byte("new image") + if _, err := s.Stage(image, rel.sign(t, image)); err != nil { + t.Fatalf("Stage: %v", err) + } + + wantLast := "rename:" + incomingRelPath + "->" + ActiveRelPath + if n := len(fs.ops); n < 2 || fs.ops[n-2] != wantLast || fs.ops[n-1] != "sync" { + t.Fatalf("ops = %v, want the activating rename immediately followed by a sync", fs.ops) + } +} + +// If the directory sync fails, Stage must say so rather than reporting +// success over a rename that might not survive a power loss. +func TestStage_ReturnsAnErrorWhenTheDirectorySyncFails(t *testing.T) { + rel := newRelease(t) + fs := newMemFS() + fs.failSync = errors.New("input/output error") + s, _ := New(fs, rel.cert) + + image := []byte("new image") + if _, err := s.Stage(image, rel.sign(t, image)); err == nil { + t.Fatal("Stage reported success despite a failed directory sync") + } +} + func TestRollback_RestoresThePreviousImage(t *testing.T) { rel := newRelease(t) fs := newMemFS() @@ -268,6 +316,47 @@ func TestRollback_RestoresThePreviousImage(t *testing.T) { } } +// Rollback's final rename needs the same directory durability barrier as +// Stage's (#373). +func TestRollback_SyncsTheESPDirectoryAfterTheRename(t *testing.T) { + rel := newRelease(t) + fs := newMemFS() + fs.files[ActiveRelPath] = []byte("old image") + s, _ := New(fs, rel.cert) + + image := []byte("bad image") + if _, err := s.Stage(image, rel.sign(t, image)); err != nil { + t.Fatalf("Stage: %v", err) + } + fs.ops = nil // only Rollback's own ops matter here + + if err := s.Rollback(); err != nil { + t.Fatalf("Rollback: %v", err) + } + + wantLast := "rename:" + incomingRelPath + "->" + ActiveRelPath + if n := len(fs.ops); n < 2 || fs.ops[n-2] != wantLast || fs.ops[n-1] != "sync" { + t.Fatalf("ops = %v, want the restoring rename immediately followed by a sync", fs.ops) + } +} + +func TestRollback_ReturnsAnErrorWhenTheDirectorySyncFails(t *testing.T) { + rel := newRelease(t) + fs := newMemFS() + fs.files[ActiveRelPath] = []byte("old image") + s, _ := New(fs, rel.cert) + + image := []byte("bad image") + if _, err := s.Stage(image, rel.sign(t, image)); err != nil { + t.Fatalf("Stage: %v", err) + } + fs.failSync = errors.New("input/output error") + + if err := s.Rollback(); err == nil { + t.Fatal("Rollback reported success despite a failed directory sync") + } +} + func TestRollback_WithNothingRetained(t *testing.T) { rel := newRelease(t) s, _ := New(newMemFS(), rel.cert) diff --git a/internal/init/imageupgrade.go b/internal/init/imageupgrade.go index 0489096..07a3715 100644 --- a/internal/init/imageupgrade.go +++ b/internal/init/imageupgrade.go @@ -36,6 +36,7 @@ import ( "log" "os" "path/filepath" + "sync" "time" nodev1 "github.com/CryptOS-PKI/cryptos-node/gen/go/cryptos/node/v1" @@ -99,6 +100,13 @@ type imageUpgradeOptions struct { // nodeImageUpgrader implements grpc.ImageUpgrader. type nodeImageUpgrader struct { opts imageUpgradeOptions + + // mu serializes Stage, Rollback and Activate (#373): all three choreograph + // the same ESP slots (BOOTX64.NEW.EFI is shared write scratch), so two of + // them in flight at once could corrupt a slot rather than merely race for + // it. TryLock rather than Lock: a caller made while another is already + // running is refused outright (FailedPrecondition), not queued behind it. + mu sync.Mutex } func newImageUpgrader(opts imageUpgradeOptions) (*nodeImageUpgrader, error) { @@ -121,6 +129,11 @@ func newImageUpgrader(opts imageUpgradeOptions) (*nodeImageUpgrader, error) { // Stage verifies the image and installs it as the one the firmware will boot, // retaining the current image. It does not reboot. func (u *nodeImageUpgrader) Stage(ctx context.Context, image, signature []byte) (*nodev1.ImageStatus, error) { + if !u.mu.TryLock() { + return nil, cgrpc.ErrUpgradeInProgress + } + defer u.mu.Unlock() + // Verified before the ESP is mounted at all, let alone writable. An image // the node cannot attribute never gets near the boot partition of a // production CA, and the stager verifies again on its own behalf -- cheap @@ -205,6 +218,11 @@ func (u *nodeImageUpgrader) reseal(ctx context.Context, esp dirFS, image []byte) // boots with, and a prune that fails does not undo the rollback: the header // keeps its old tokens, the rollback target among them, until the next stage. func (u *nodeImageUpgrader) Rollback(ctx context.Context) (*nodev1.ImageStatus, error) { + if !u.mu.TryLock() { + return nil, cgrpc.ErrUpgradeInProgress + } + defer u.mu.Unlock() + var st imageupgrade.Status err := u.opts.Mount(true, func(root string) error { esp := dirFS{root: root} @@ -298,6 +316,11 @@ func (u *nodeImageUpgrader) pruneAfterRollback(ctx context.Context, esp dirFS, r // boot the same image the node is already running is an outage with nothing to // show for it -- far more likely a mistake than an intention. func (u *nodeImageUpgrader) Activate(ctx context.Context, confirmCommonName string) error { + if !u.mu.TryLock() { + return cgrpc.ErrUpgradeInProgress + } + defer u.mu.Unlock() + if err := reset.CheckConfirm(u.opts.CACN(), confirmCommonName); err != nil { return err } @@ -397,6 +420,23 @@ func (d dirFS) WriteFile(rel string, data []byte) error { return f.Close() } +// Sync fsyncs the ESP root directory, the barrier a vfat rename needs: the +// rename that swaps the active image updates a directory entry, which a +// file's own fsync (WriteFile, above) does not cover. +func (d dirFS) Sync() error { + f, err := os.Open(d.root) + if err != nil { + return err + } + if err := f.Sync(); err != nil { + _ = f.Close() + + return err + } + + return f.Close() +} + // digestFile returns the SHA-256 (hex) of rel under a mounted ESP, or an empty // string when it is not there. It is how a booting node learns which image it // is running: at startup the file on the boot path is, by definition, the one diff --git a/internal/init/imageupgrade_test.go b/internal/init/imageupgrade_test.go index 868f6d3..6b2055e 100644 --- a/internal/init/imageupgrade_test.go +++ b/internal/init/imageupgrade_test.go @@ -333,6 +333,113 @@ func TestUpgrader_ActivateRebootsOnTheRightCommonName(t *testing.T) { } } +// Stage, Rollback and Activate share one lock (#373): two upgrade calls at +// the same time could otherwise both write BOOTX64.NEW.EFI, and a caller made +// while another is already running gets FailedPrecondition rather than +// queuing behind it or racing it. +// +// This drives the overlap with real goroutines rather than only holding the +// lock by hand, so it proves the lock is actually wired into Stage, not just +// that TryLock itself works. +func TestUpgrader_StageSerializesAgainstAConcurrentStage(t *testing.T) { + rel := newReleaseKey(t) + old := []byte("the running image") + esp := newESPDir(t, old) + started := make(chan struct{}) + release := make(chan struct{}) + blockingMount := func(readWrite bool, fn func(root string) error) error { + return esp.mount(readWrite, func(root string) error { + close(started) + <-release + + return fn(root) + }) + } + u, err := newImageUpgrader(imageUpgradeOptions{ + CACN: func() string { return testCACN }, + Mount: blockingMount, + Reboot: func() { t.Fatal("must not reboot") }, + Release: rel.cert, + Running: digestOf(old), + Version: "v1.2.3", + }) + if err != nil { + t.Fatalf("newImageUpgrader: %v", err) + } + + image := []byte("the new image") + sig := rel.sign(t, image) + firstDone := make(chan error, 1) + go func() { + _, stageErr := u.Stage(context.Background(), image, sig) + firstDone <- stageErr + }() + + <-started + if _, err := u.Stage(context.Background(), image, sig); !errors.Is(err, grpc.ErrUpgradeInProgress) { + t.Fatalf("concurrent Stage err = %v, want ErrUpgradeInProgress", err) + } + close(release) + if err := <-firstDone; err != nil { + t.Fatalf("the first Stage: %v", err) + } +} + +// A caller must not be able to Stage while a Rollback the test holds the lock +// for is in flight (or vice versa); checked here against the lock directly, +// which exercises the same guard the goroutine test above exercises through +// real concurrency. +func TestUpgrader_StageRefusesWhileAnUpgradeIsInProgress(t *testing.T) { + rel := newReleaseKey(t) + old := []byte("the running image") + esp := newESPDir(t, old) + u := newTestUpgrader(t, esp, rel, old, func() { t.Fatal("must not reboot") }) + u.mu.Lock() + defer u.mu.Unlock() + + image := []byte("the new image") + if _, err := u.Stage(context.Background(), image, rel.sign(t, image)); !errors.Is(err, grpc.ErrUpgradeInProgress) { + t.Fatalf("err = %v, want ErrUpgradeInProgress", err) + } + if esp.mountedReadWrite() { + t.Error("a refused concurrent Stage still mounted the ESP") + } +} + +func TestUpgrader_RollbackRefusesWhileAnUpgradeIsInProgress(t *testing.T) { + rel := newReleaseKey(t) + old := []byte("the running image") + esp := newESPDir(t, old) + u := newTestUpgrader(t, esp, rel, old, func() { t.Fatal("must not reboot") }) + u.mu.Lock() + defer u.mu.Unlock() + + if _, err := u.Rollback(context.Background()); !errors.Is(err, grpc.ErrUpgradeInProgress) { + t.Fatalf("err = %v, want ErrUpgradeInProgress", err) + } + if esp.mountedReadWrite() { + t.Error("a refused concurrent Rollback still mounted the ESP") + } +} + +func TestUpgrader_ActivateRefusesWhileAnUpgradeIsInProgress(t *testing.T) { + rel := newReleaseKey(t) + old := []byte("the running image") + esp := newESPDir(t, old) + u := newTestUpgrader(t, esp, rel, old, func() { t.Fatal("must not reboot") }) + + image := []byte("the new image") + if _, err := u.Stage(context.Background(), image, rel.sign(t, image)); err != nil { + t.Fatalf("Stage: %v", err) + } + u.mu.Lock() + defer u.mu.Unlock() + + if err := u.Activate(context.Background(), testCACN); !errors.Is(err, grpc.ErrUpgradeInProgress) { + t.Fatalf("err = %v, want ErrUpgradeInProgress", err) + } +} + // On a node that has not been upgraded, running and active are the same image // and nothing is outstanding. func TestUpgrader_StatusOnAnUnupgradedNode(t *testing.T) {