From c86b67d362b21b98fd1af097162c012a345504b0 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 23 Aug 2026 11:56:39 -0700 Subject: [PATCH] fix: make Cloud Hypervisor writable overlays privately propagated Selective `filesystem.allowWrite` never worked on a real host. The live-KVM job fails every selective run before the guest boots: [cloud-hypervisor] stage=vmm-configuration status=failed Staged mount tree propagation would leak: /run/awf-cloud-hypervisor/virtiofsd/awf-.../0-workspace/allowed has shared:1 `stageReadonlyRoot()` makes the staged root `--make-rprivate`, but that only covers mounts existing at that moment. `stageWritableOverlays()` then creates new bind mounts, and a bind mount joins the *source's* peer group -- not the destination parent's. Because the workspace lives under a shared mount on any systemd host (including GitHub-hosted runners), each overlay arrived `shared:N`, and `assertPrivatePropagation()` correctly refused to stage the tree. Make each overlay private immediately after binding it, before the writable remount. `--make-rprivate` is already required by `assertMountToolSupported()` (util-linux >= 2.23), so this adds no new tool requirement, and the overlay bind stays non-recursive. The unit tests missed this because the fake mount table modelled `mount --bind` as producing an already-private mount. Correct the fake to model the kernel's actual peer-group inheritance, which reproduces the exact production error, and add a named regression test. Verified against a real kernel: with the fix, a selective directory overlay, a selective file overlay, and a zero-overlay read-only tree all stage cleanly, allowed writes persist to the host, sibling/parent/create/ truncate/rename/delete outside allowWrite are denied, and teardown leaves no residue. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 248b577f-2e6f-4e03-90bc-96c75c0d395e --- src/cloud-hypervisor/mount-tree.test.ts | 36 +++++++++++++++++++++++-- src/cloud-hypervisor/mount-tree.ts | 9 +++++++ src/cloud-hypervisor/virtiofsd.test.ts | 1 + 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/cloud-hypervisor/mount-tree.test.ts b/src/cloud-hypervisor/mount-tree.test.ts index b87bef007..4471dae8a 100644 --- a/src/cloud-hypervisor/mount-tree.test.ts +++ b/src/cloud-hypervisor/mount-tree.test.ts @@ -64,7 +64,15 @@ function mountTable(options: { ineffectiveRemount?: boolean; shared?: boolean } return; } if (args[0] === '--bind') { - table.set(args[2], { options: ['rw', 'relatime'], optionalFields: [] }); + // A new bind mount joins the *source's* peer group, so binding from a + // shared host mount yields a shared mount even when the destination's + // parent is already private. Modelling that is what makes the overlay's + // explicit `--make-rprivate` observable here instead of only on a real + // kernel. + table.set(args[2], { + options: ['rw', 'relatime'], + optionalFields: options.shared === false ? [] : ['shared:23'], + }); return; } if (args[0] === '--make-rprivate') { @@ -275,6 +283,7 @@ describe('StagedHostMountTree', () => { await staged.stage(); expect(fake.commands.slice(4)).toEqual([ [tools.mount, '--bind', '/host/workspace/out', `${ROOT}/out`], + [tools.mount, '--make-rprivate', `${ROOT}/out`], [tools.mount, '-o', 'remount,bind,rw,nosuid,nodev', `${ROOT}/out`], [ tools.mount, @@ -282,6 +291,7 @@ describe('StagedHostMountTree', () => { '/host/workspace/deep/nested/state.json', `${ROOT}/deep/nested/state.json`, ], + [tools.mount, '--make-rprivate', `${ROOT}/deep/nested/state.json`], [ tools.mount, '-o', @@ -295,6 +305,26 @@ describe('StagedHostMountTree', () => { expect(fake.table.get(`${ROOT}/nested`)?.options).toContain('ro'); }); + it('makes each writable overlay privately propagated so it cannot leak to the host', async () => { + // Regression: overlays were previously bound without being made private. + // A bind mount joins the source's peer group, so on any host where the + // workspace lives under a shared mount -- the default under systemd, and + // what GitHub-hosted runners provide -- every selective `allowWrite` run + // aborted with "Staged mount tree propagation would leak". + const fake = mountTable(); + const staged = tree( + fake, + plan([ + { source: '/host/workspace/out', destination: '/host/workspace/out', kind: 'directory' }, + ]), + ); + await expect(staged.stage()).resolves.toBeUndefined(); + expect(fake.commands).toContainEqual([tools.mount, '--make-rprivate', `${ROOT}/out`]); + expect(fake.table.get(`${ROOT}/out`)?.optionalFields).toEqual([]); + expect(fake.table.get(`${ROOT}/out`)?.options).toContain('rw'); + expect(fake.table.get(ROOT)?.options).toContain('ro'); + }); + it('unmounts children deepest-first and the staged root last', async () => { const fake = mountTable(); const staged = tree( @@ -315,7 +345,9 @@ describe('StagedHostMountTree', () => { ROOT, ]); await staged.unmount(); - expect(fake.commands.slice(8)).toEqual([ + // Sliced from the end so the assertion stays about unmount ordering rather + // than the exact number of staging commands that preceded it. + expect(fake.commands.slice(-3)).toEqual([ [tools.umount, `${ROOT}/deep/nested/state.json`], [tools.umount, `${ROOT}/out`], [tools.umount, '-R', ROOT], diff --git a/src/cloud-hypervisor/mount-tree.ts b/src/cloud-hypervisor/mount-tree.ts index 73c69f02f..18bb14696 100644 --- a/src/cloud-hypervisor/mount-tree.ts +++ b/src/cloud-hypervisor/mount-tree.ts @@ -266,6 +266,15 @@ export class StagedHostMountTree { overlay.stagedDestination, ]); this.pendingMounts.add(overlay.stagedDestination); + // A new bind mount joins the *source's* peer group, so an overlay bound + // from a shared host mount (the default for `/` under systemd, and what + // GitHub-hosted runners provide) arrives shared even though the staged + // root was already made private. Making the root private beforehand only + // covers mounts that existed at that point, so each overlay has to be + // made private in turn -- otherwise the writable overlay would propagate + // back into the host peer group and `assertPrivatePropagation()` would + // (correctly) refuse to stage the tree at all. + await dependencies.runTool(tools.mount, ['--make-rprivate', overlay.stagedDestination]); await dependencies.runTool(tools.mount, [ '-o', WRITABLE_REMOUNT_OPTIONS, diff --git a/src/cloud-hypervisor/virtiofsd.test.ts b/src/cloud-hypervisor/virtiofsd.test.ts index 5f8cfec41..8df96fa50 100644 --- a/src/cloud-hypervisor/virtiofsd.test.ts +++ b/src/cloud-hypervisor/virtiofsd.test.ts @@ -284,6 +284,7 @@ describe('VirtiofsdManager host mount-tree enforcement', () => { ['/usr/bin/mount', ['--make-rprivate', STAGED_ROOT]], ['/usr/bin/mount', ['-o', 'remount,bind,ro,nosuid,nodev', STAGED_ROOT]], ['/usr/bin/mount', ['--bind', '/host/workspace/out', `${STAGED_ROOT}/out`]], + ['/usr/bin/mount', ['--make-rprivate', `${STAGED_ROOT}/out`]], ['/usr/bin/mount', ['-o', 'remount,bind,rw,nosuid,nodev', `${STAGED_ROOT}/out`]], ]); expect(deps.mkdir).toHaveBeenCalledWith(STAGED_ROOT, { recursive: true, mode: 0o700 });