From bcb5ccc005568ada0e3541df01d31e3d53f0c0db Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Wed, 14 Oct 2026 15:30:50 -0700 Subject: [PATCH 1/2] fix: preserve durable KVM runner ACL Retain the first validated device ACL baseline after launch intent completion. Compose its non-VMM entries with fresh deletion baselines during normal and stale cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b641e35-ee0f-404f-b119-2dd2ebe950fa --- docs/cloud-hypervisor-foundation.md | 10 +++-- src/cloud-hypervisor/cleanup-handle.test.ts | 14 +++++++ src/cloud-hypervisor/cleanup-handle.ts | 4 ++ src/cloud-hypervisor/cleanup-identity.test.ts | 41 +++++++++++++++++++ src/cloud-hypervisor/cleanup-identity.ts | 40 ++++++++++++++++++ src/cloud-hypervisor/cleanup-registry.test.ts | 37 +++++++++-------- .../cleanup-vmm-identity.test.ts | 33 +++++++++++---- src/cloud-hypervisor/cleanup-vmm-identity.ts | 14 +++++++ src/cloud-hypervisor/device-acl.test.ts | 36 ++++++++++++++++ src/cloud-hypervisor/device-acl.ts | 33 +++++++++++++-- src/cloud-hypervisor/vmm-identity.test.ts | 9 ++-- src/cloud-hypervisor/vmm-identity.ts | 12 ++++++ 12 files changed, 246 insertions(+), 37 deletions(-) diff --git a/docs/cloud-hypervisor-foundation.md b/docs/cloud-hypervisor-foundation.md index 6b629d406..5941f4404 100644 --- a/docs/cloud-hypervisor-foundation.md +++ b/docs/cloud-hypervisor-foundation.md @@ -144,9 +144,13 @@ becomes live. The host bridge-forwarding rule is tagged with a per-run iptables comment and recorded by its exact tuple, so concurrent runs do not share an anonymously owned rule. Dedicated VMM account and device-ACL intent is persisted before `useradd` -or `setfacl` runs. Recovery validates the account's random name, run-specific -passwd metadata, numeric uid/gid, and exact numeric ACL entries before removing -them. +or `setfacl` runs. The durable record retains the first validated ACL baseline +for every accessed device after launch-time revocation completes. Account +deletion still journals a fresh baseline under the shared device lock, but +restoration composes it with those durable non-VMM entries so runner access, +mask semantics, and live peer VMM entries survive normal or stale cleanup. +Recovery validates the account's random name, run-specific passwd metadata, +numeric uid/gid, and exact numeric ACL entries before removing them. Any staged virtio-fs bind mounts are recorded by mount ID, device, root, target, filesystem type, and source; stale recovery revalidates and unmounts them deepest-first before removing their inode-validated share directory. diff --git a/src/cloud-hypervisor/cleanup-handle.test.ts b/src/cloud-hypervisor/cleanup-handle.test.ts index b277c9413..aa7dc0d6b 100644 --- a/src/cloud-hypervisor/cleanup-handle.test.ts +++ b/src/cloud-hypervisor/cleanup-handle.test.ts @@ -42,10 +42,18 @@ describe('cleanup handle integration', () => { vmmIdentity: { accessedAclPaths?: string[]; aclTemporaryUids?: Record; + accessedAclBaselines?: Record; + accessedAclTemporaryUids?: Record; }; }; expect(preparedRecord.vmmIdentity.accessedAclPaths).toEqual(['/dev/kvm']); expect(preparedRecord.vmmIdentity.aclTemporaryUids).toEqual({ '/dev/kvm': temporaryUids }); + expect(preparedRecord.vmmIdentity.accessedAclBaselines).toEqual({ + '/dev/kvm': baseline, + }); + expect(preparedRecord.vmmIdentity.accessedAclTemporaryUids).toEqual({ + '/dev/kvm': temporaryUids, + }); await handle.releaseVmmAcl('/dev/kvm'); @@ -55,12 +63,18 @@ describe('cleanup handle integration', () => { accessedAclPaths?: string[]; aclBaselines?: object; aclTemporaryUids?: object; + accessedAclBaselines?: object; + accessedAclTemporaryUids?: object; }; }; expect(record.vmmIdentity.aclPaths).toEqual([]); expect(record.vmmIdentity.accessedAclPaths).toEqual(['/dev/kvm']); expect(record.vmmIdentity.aclBaselines).toEqual({}); expect(record.vmmIdentity.aclTemporaryUids).toEqual({}); + expect(record.vmmIdentity.accessedAclBaselines).toEqual({ '/dev/kvm': baseline }); + expect(record.vmmIdentity.accessedAclTemporaryUids).toEqual({ + '/dev/kvm': temporaryUids, + }); await expect(handle.releaseVmmAcl('/dev/kvm')).rejects.toThrow(/intent is missing/); }); diff --git a/src/cloud-hypervisor/cleanup-handle.ts b/src/cloud-hypervisor/cleanup-handle.ts index 7758150d0..fe033ed98 100644 --- a/src/cloud-hypervisor/cleanup-handle.ts +++ b/src/cloud-hypervisor/cleanup-handle.ts @@ -118,6 +118,10 @@ export function createCleanupHandle(options: CleanupHandleFactoryOptions): Cloud if (!record.vmmIdentity.accessedAclPaths.includes(aclPath)) { record.vmmIdentity.accessedAclPaths.push(aclPath); } + record.vmmIdentity.accessedAclBaselines ??= {}; + record.vmmIdentity.accessedAclTemporaryUids ??= {}; + record.vmmIdentity.accessedAclBaselines[aclPath] ??= validatedBaseline; + record.vmmIdentity.accessedAclTemporaryUids[aclPath] ??= [...temporaryUids]; if (!record.vmmIdentity.aclPaths.includes(aclPath)) { record.vmmIdentity.aclPaths.push(aclPath); record.vmmIdentity.aclBaselines ??= {}; diff --git a/src/cloud-hypervisor/cleanup-identity.test.ts b/src/cloud-hypervisor/cleanup-identity.test.ts index cfcf416ab..2b8036f74 100644 --- a/src/cloud-hypervisor/cleanup-identity.test.ts +++ b/src/cloud-hypervisor/cleanup-identity.test.ts @@ -103,6 +103,47 @@ describe('cleanup identity primitives', () => { accessedAclPaths: ['/dev/other'], }; }, /cleanup VMM identity is malformed/], + ['bad-accessed-acl-baseline-path', (record) => { + record.vmmIdentity = { + state: 'live', + name: 'awfvmm-0123456789abcdef0123', + uid: 23001, + gid: 23002, + aclPaths: [], + accessedAclPaths: ['/dev/kvm'], + accessedAclBaselines: { + '/dev/net/tun': ['user::rw-', 'group::rw-', 'other::---'], + }, + }; + }, /cleanup VMM identity is malformed/], + ['bad-accessed-acl-temporary-uid', (record) => { + record.vmmIdentity = { + state: 'live', + name: 'awfvmm-0123456789abcdef0123', + uid: 23001, + gid: 23002, + aclPaths: [], + accessedAclPaths: ['/dev/kvm'], + accessedAclBaselines: { + '/dev/kvm': [ + 'user::rw-', 'user:22001:rw-', + 'group::rw-', 'mask::rw-', 'other::---', + ], + }, + accessedAclTemporaryUids: { '/dev/kvm': [22002] }, + }; + }, /cleanup VMM identity is malformed/], + ['accessed-acl-temporary-list-without-baseline', (record) => { + record.vmmIdentity = { + state: 'live', + name: 'awfvmm-0123456789abcdef0123', + uid: 23001, + gid: 23002, + aclPaths: [], + accessedAclPaths: ['/dev/kvm'], + accessedAclTemporaryUids: { '/dev/kvm': [] }, + }; + }, /cleanup VMM identity is malformed/], ['pending-acl-not-accessed', (record) => { record.vmmIdentity = { state: 'live', diff --git a/src/cloud-hypervisor/cleanup-identity.ts b/src/cloud-hypervisor/cleanup-identity.ts index 519993199..321a3f0fc 100644 --- a/src/cloud-hypervisor/cleanup-identity.ts +++ b/src/cloud-hypervisor/cleanup-identity.ts @@ -102,6 +102,8 @@ export interface CleanupRecord { accessedAclPaths?: CloudHypervisorDevicePath[]; aclBaselines?: Partial>; aclTemporaryUids?: Partial>; + accessedAclBaselines?: Partial>; + accessedAclTemporaryUids?: Partial>; }; mounts: MountIdentity[]; updatedAt: string; @@ -230,6 +232,44 @@ export function validateRecord( identity.aclBaselines?.[aclPath as CloudHypervisorDevicePath] ?.includes(`user:${uid}:rw-`))) )) || + (identity.accessedAclBaselines !== undefined && ( + typeof identity.accessedAclBaselines !== 'object' || + identity.accessedAclBaselines === null || + Array.isArray(identity.accessedAclBaselines) || + Object.keys(identity.accessedAclBaselines).some((aclPath) => + aclPath !== '/dev/kvm' && aclPath !== '/dev/net/tun') || + Object.entries(identity.accessedAclBaselines).some(([aclPath, baseline]) => { + try { + return !identity.accessedAclPaths?.includes( + aclPath as CloudHypervisorDevicePath, + ) || + !Array.isArray(baseline) || + parseDeviceAcl(baseline.join('\n')).length !== baseline.length; + } catch { + return true; + } + }) + )) || + (identity.accessedAclTemporaryUids !== undefined && ( + typeof identity.accessedAclTemporaryUids !== 'object' || + identity.accessedAclTemporaryUids === null || + Array.isArray(identity.accessedAclTemporaryUids) || + Object.keys(identity.accessedAclTemporaryUids).some((aclPath) => + (aclPath !== '/dev/kvm' && aclPath !== '/dev/net/tun') || + identity.accessedAclBaselines?.[ + aclPath as CloudHypervisorDevicePath + ] === undefined) || + Object.entries(identity.accessedAclTemporaryUids).some(([aclPath, uids]) => + !identity.accessedAclPaths?.includes( + aclPath as CloudHypervisorDevicePath, + ) || + !Array.isArray(uids) || + uids.some((uid) => !Number.isSafeInteger(uid) || uid <= 0) || + new Set(uids).size !== uids.length || + !uids.every((uid) => + identity.accessedAclBaselines?.[aclPath as CloudHypervisorDevicePath] + ?.includes(`user:${uid}:rw-`))) + )) || (identity.state === 'live' && (() => { const uid = identity.uid; const gid = identity.gid; diff --git a/src/cloud-hypervisor/cleanup-registry.test.ts b/src/cloud-hypervisor/cleanup-registry.test.ts index 21596e22a..c9f21327a 100644 --- a/src/cloud-hypervisor/cleanup-registry.test.ts +++ b/src/cloud-hypervisor/cleanup-registry.test.ts @@ -58,7 +58,7 @@ describe('DurableCloudHypervisorCleanupRegistry orchestration', () => { expect((await fs.stat(path.dirname(recordPath))).mode & 0o777).toBe(0o700); }); - it('restores the exact /dev/kvm ACL baseline after run 38474851931 forced termination', async () => { + it('restores the durable runner ACE during stale recovery after launch intent release', async () => { const paths = harness.runPaths('vmm-account-recovery'); const account = 'awfvmm-0123456789abcdef0123'; const recordPath = path.join( @@ -68,7 +68,15 @@ describe('DurableCloudHypervisorCleanupRegistry orchestration', () => { ); let userExists = true; let groupExists = true; - let aclExists = true; + const runnerBaseline = [ + 'user::rw-', 'user:1001:rw-', + 'group::rw-', 'mask::rw-', 'other::rw-', + ]; + const maskOnly = ['user::rw-', 'group::rw-', 'mask::rw-', 'other::rw-']; + const deviceAcls = new Map([ + ['/dev/kvm', [...maskOnly]], + ['/dev/net/tun', [...maskOnly]], + ]); const tools = { getfacl: '/usr/bin/getfacl', getent: '/usr/bin/getent', @@ -96,18 +104,9 @@ describe('DurableCloudHypervisorCleanupRegistry orchestration', () => { : { exitCode: 2, stdout: '', stderr: '' }; } if (command === tools.getfacl) { - if (args[args.length - 1] === '/dev/net/tun') { - return { - exitCode: 0, - stdout: 'user::rw-\ngroup::rw-\nmask::rw-\nother::rw-\n', - stderr: '', - }; - } return { exitCode: 0, - stdout: aclExists - ? 'user::rw-\nuser:23001:rw-\ngroup::rw-\nmask::rw-\nother::rw-\n' - : 'user::rw-\ngroup::rw-\nmask::rw-\nother::rw-\n', + stdout: `${deviceAcls.get(args[args.length - 1])!.join('\n')}\n`, stderr: '', }; } @@ -116,12 +115,12 @@ describe('DurableCloudHypervisorCleanupRegistry orchestration', () => { if (args[0] === '-u') return { exitCode: 0, stdout: '23001\n', stderr: '' }; return { exitCode: 0, stdout: '23002\n', stderr: '' }; } - if (command === tools.setfacl) { + if (command === tools.setfacl && args[0].startsWith('--set=')) { await expect(fs.access(path.join( harness.temporaryRoot, '.device-acl-lock', ))).resolves.toBeUndefined(); - aclExists = false; + deviceAcls.set(args[1], args[0].slice('--set='.length).split(',')); } if (command === tools.userdel) { const persisted = JSON.parse(await fs.readFile(recordPath, 'utf8')) as { @@ -140,18 +139,20 @@ describe('DurableCloudHypervisorCleanupRegistry orchestration', () => { await handle.captureArtifactSnapshot(snapshot); await handle.prepareVmmAccount(account); await handle.captureVmmIdentity({ name: account, uid: 23001, gid: 23002 }); - const baseline = ['user::rw-', 'group::rw-', 'mask::rw-', 'other::rw-']; - await handle.prepareVmmAcl('/dev/kvm', baseline); - await handle.prepareVmmAcl('/dev/net/tun', baseline); + await handle.prepareVmmAcl('/dev/kvm', runnerBaseline); + await handle.prepareVmmAcl('/dev/net/tun', runnerBaseline); + await handle.releaseVmmAcl('/dev/kvm'); await handle.releaseVmmAcl('/dev/net/tun'); harness.state.ownerStartTime = '2000'; await registry.reapPending(tools.ip, '/usr/bin/umount', tools); expect(run).toHaveBeenCalledWith(tools.setfacl, [ - `--set=${baseline.join(',')}`, + `--set=${runnerBaseline.join(',')}`, '/dev/kvm', ]); + expect(deviceAcls.get('/dev/kvm')).toEqual(runnerBaseline); + expect(deviceAcls.get('/dev/net/tun')).toEqual(runnerBaseline); expect(run).toHaveBeenCalledWith(tools.userdel, [account]); expect(run).toHaveBeenCalledWith(tools.groupdel, [account]); await expect(fs.access(snapshot)).rejects.toMatchObject({ code: 'ENOENT' }); diff --git a/src/cloud-hypervisor/cleanup-vmm-identity.test.ts b/src/cloud-hypervisor/cleanup-vmm-identity.test.ts index 02cd859fa..68e607913 100644 --- a/src/cloud-hypervisor/cleanup-vmm-identity.test.ts +++ b/src/cloud-hypervisor/cleanup-vmm-identity.test.ts @@ -34,6 +34,8 @@ function cleanupRecord( aclPaths?: Array<'/dev/kvm' | '/dev/net/tun'>; accessedAclPaths?: Array<'/dev/kvm' | '/dev/net/tun'>; aclTemporaryUids?: Partial>; + accessedAclBaselines?: Partial>; + accessedAclTemporaryUids?: Partial>; } = {}, ): CleanupRecord { const runId = options.runId ?? 'acl-recovery'; @@ -70,6 +72,12 @@ function cleanupRecord( ...(options.aclTemporaryUids ? { aclTemporaryUids: options.aclTemporaryUids, } : {}), + ...(options.accessedAclBaselines ? { + accessedAclBaselines: options.accessedAclBaselines, + } : {}), + ...(options.accessedAclTemporaryUids ? { + accessedAclTemporaryUids: options.accessedAclTemporaryUids, + } : {}), }, mounts: [], updatedAt: new Date(0).toISOString(), @@ -191,15 +199,16 @@ describe('Cloud Hypervisor durable VMM identity cleanup', () => { }); it.each([ - ['crash after launch restoration', [], false], - ['partial launch-intent release', ['/dev/kvm'], false], - ['partial preparation retry', [], true], + ['cancellation recovery after launch restoration', [], false], + ['stale recovery after partial launch-intent release', ['/dev/kvm'], false], + ['partial preparation persistence retry', [], true], ] as const)( - 'journals fresh baselines for every accessed device before account deletion: %s', + 'preserves durable runner ACL history while journaling fresh deletion baselines: %s', async (_name, pendingPaths, retryAfterPersistFailure) => { + const maskOnly = ['user::rw-', 'group::rw-', 'mask::rw-', 'other::rw-']; const deviceAcls = new Map([ - ['/dev/kvm', [...baseline]], - ['/dev/net/tun', [...baseline]], + ['/dev/kvm', [...maskOnly]], + ['/dev/net/tun', [...maskOnly]], ]); let accountExists = true; let groupExists = true; @@ -209,6 +218,14 @@ describe('Cloud Hypervisor durable VMM identity cleanup', () => { aclPaths: [...pendingPaths], accessedAclPaths: ['/dev/kvm', '/dev/net/tun'], aclTemporaryUids: pendingPaths.length > 0 ? { '/dev/kvm': [] } : undefined, + accessedAclBaselines: { + '/dev/kvm': baseline, + '/dev/net/tun': baseline, + }, + accessedAclTemporaryUids: { + '/dev/kvm': [], + '/dev/net/tun': [], + }, }, ); const run = jest.fn(async (command: string, args: readonly string[]) => { @@ -285,8 +302,8 @@ describe('Cloud Hypervisor durable VMM identity cleanup', () => { expect(record.vmmIdentity?.aclPaths).toEqual(['/dev/kvm', '/dev/net/tun']); expect(record.vmmIdentity?.aclBaselines).toEqual({ - '/dev/kvm': baseline, - '/dev/net/tun': baseline, + '/dev/kvm': pendingPaths.length > 0 ? baseline : maskOnly, + '/dev/net/tun': maskOnly, }); expect(persist).toHaveBeenCalledTimes( 2 - pendingPaths.length + (retryAfterPersistFailure ? 1 : 0), diff --git a/src/cloud-hypervisor/cleanup-vmm-identity.ts b/src/cloud-hypervisor/cleanup-vmm-identity.ts index 04e9d25af..94a41c29b 100644 --- a/src/cloud-hypervisor/cleanup-vmm-identity.ts +++ b/src/cloud-hypervisor/cleanup-vmm-identity.ts @@ -80,12 +80,20 @@ async function prepareMissingVmmAclsLocked( identity.aclBaselines[aclPath] = baseline; identity.aclTemporaryUids ??= {}; identity.aclTemporaryUids[aclPath] = temporaryUids; + identity.accessedAclBaselines ??= {}; + identity.accessedAclTemporaryUids ??= {}; + identity.accessedAclBaselines[aclPath] ??= baseline; + identity.accessedAclTemporaryUids[aclPath] ??= temporaryUids; try { if (persist) await persist(); } catch (error) { identity.aclPaths.pop(); delete identity.aclBaselines[aclPath]; delete identity.aclTemporaryUids[aclPath]; + if (identity.accessedAclBaselines?.[aclPath] === baseline) { + delete identity.accessedAclBaselines[aclPath]; + delete identity.accessedAclTemporaryUids?.[aclPath]; + } throw error; } if (baseline.some((entry) => entry.startsWith(`user:${identity.uid}:`))) { @@ -152,6 +160,12 @@ async function restoreVmmAclsLocked( identity.uid!, identity.aclTemporaryUids?.[aclPath as CloudHypervisorDevicePath] ?? [], liveTemporaryUids, + identity.accessedAclBaselines?.[aclPath as CloudHypervisorDevicePath] ?? baseline, + identity.accessedAclTemporaryUids?.[ + aclPath as CloudHypervisorDevicePath + ] ?? identity.aclTemporaryUids?.[ + aclPath as CloudHypervisorDevicePath + ] ?? [], ); } restoredTargets.set(aclPath as CloudHypervisorDevicePath, restoredTarget); diff --git a/src/cloud-hypervisor/device-acl.test.ts b/src/cloud-hypervisor/device-acl.test.ts index 610da5848..90b2615ac 100644 --- a/src/cloud-hypervisor/device-acl.test.ts +++ b/src/cloud-hypervisor/device-acl.test.ts @@ -117,6 +117,42 @@ describe('Cloud Hypervisor device ACLs', () => { ]); }); + it('restores a durable runner ACE from accessed history without dropping mask residue', () => { + expect(composeDeviceAcl( + ['user::rw-', 'group::rw-', 'mask::rw-', 'other::---'], + ['user::rw-', 'group::rw-', 'mask::rw-', 'other::---'], + 999, + [], + [], + [ + 'user::rw-', 'user:1001:rw-', + 'group::rw-', 'mask::rw-', 'other::---', + ], + [], + )).toEqual([ + 'user::rw-', 'user:1001:rw-', + 'group::rw-', 'mask::rw-', 'other::---', + ]); + }); + + it('retains the broader historical mask required by a durable runner ACE', () => { + expect(composeDeviceAcl( + ['user::rw-', 'group::r--', 'mask::r--', 'other::---'], + ['user::rw-', 'group::r--', 'mask::r--', 'other::---'], + 999, + [], + [], + [ + 'user::rw-', 'user:1001:rw-', + 'group::r--', 'mask::rw-', 'other::---', + ], + [], + )).toEqual([ + 'user::rw-', 'user:1001:rw-', + 'group::r--', 'mask::rw-', 'other::---', + ]); + }); + it('rejects a named ACE that is neither durable nor a live VMM peer', () => { expect(() => composeDeviceAcl( [ diff --git a/src/cloud-hypervisor/device-acl.ts b/src/cloud-hypervisor/device-acl.ts index b836a8546..174942c1f 100644 --- a/src/cloud-hypervisor/device-acl.ts +++ b/src/cloud-hypervisor/device-acl.ts @@ -60,9 +60,12 @@ export function composeDeviceAcl( ownerUid: number, capturedTemporaryUids: readonly number[], liveTemporaryUids: readonly number[], + accessedBaseline: readonly string[] = baseline, + accessedTemporaryUids: readonly number[] = capturedTemporaryUids, ): string[] { parseDeviceAcl(baseline.join('\n')); parseDeviceAcl(current.join('\n')); + parseDeviceAcl(accessedBaseline.join('\n')); const capturedTemporary = new Set(capturedTemporaryUids); const liveTemporary = new Set(liveTemporaryUids); capturedTemporary.add(ownerUid); @@ -77,6 +80,14 @@ export function composeDeviceAcl( const uid = namedUserAclUid(entry); if (uid !== undefined) durableNamedUsers.set(uid, entry); } + const accessedTemporary = new Set(accessedTemporaryUids); + accessedTemporary.add(ownerUid); + for (const entry of accessedBaseline) { + const uid = namedUserAclUid(entry); + if (uid !== undefined && !accessedTemporary.has(uid) && !durableNamedUsers.has(uid)) { + durableNamedUsers.set(uid, entry); + } + } const temporaryNamedUsers = new Map(); for (const entry of current) { const uid = namedUserAclUid(entry); @@ -103,14 +114,28 @@ export function composeDeviceAcl( ...withoutNamedUsers.slice(ownerIndex + 1), ]; - if (temporaryNamedUsers.size > 0) { - const currentMask = current.find((entry) => entry.startsWith('mask::'))!; + if (temporaryNamedUsers.size > 0 || durableNamedUsers.size > 0) { + const currentMask = current.find((entry) => entry.startsWith('mask::')); + const accessedMask = accessedBaseline.find((entry) => entry.startsWith('mask::')); + const requiredMask = mergeAclMasks(currentMask, accessedMask); const maskIndex = composed.findIndex((entry) => entry.startsWith('mask::')); - if (maskIndex >= 0) composed[maskIndex] = currentMask; + if (maskIndex >= 0) composed[maskIndex] = requiredMask; else { const otherIndex = composed.findIndex((entry) => entry.startsWith('other::')); - composed.splice(otherIndex, 0, currentMask); + composed.splice(otherIndex, 0, requiredMask); } } return parseDeviceAcl(composed.join('\n')); } + +function mergeAclMasks( + currentMask: string | undefined, + accessedMask: string | undefined, +): string { + const permissions = [0, 1, 2].map((index) => { + const current = currentMask?.slice(-3)[index] ?? '-'; + const accessed = accessedMask?.slice(-3)[index] ?? '-'; + return current !== '-' ? current : accessed; + }).join(''); + return `mask::${permissions}`; +} diff --git a/src/cloud-hypervisor/vmm-identity.test.ts b/src/cloud-hypervisor/vmm-identity.test.ts index db262acd6..130adf0d0 100644 --- a/src/cloud-hypervisor/vmm-identity.test.ts +++ b/src/cloud-hypervisor/vmm-identity.test.ts @@ -515,10 +515,10 @@ describe('CloudHypervisorVmmIdentityManager', () => { ]; return { stdout: '', stderr: '' }; } - if (command === tools.setfacl) { + if (command === tools.setfacl && args[0].startsWith('--set=')) { expect(deviceLockHeld).toBe(true); lockedCommands.push('setfacl'); - currentAcl = [...runnerBaseline]; + currentAcl = args[0].slice('--set='.length).split(','); return { stdout: '', stderr: '' }; } if (command === tools.userdel) { @@ -549,6 +549,7 @@ describe('CloudHypervisorVmmIdentityManager', () => { await manager.allocate(); await manager.withDeviceAccess(async () => undefined, false); expect(observer.releaseAcl).toHaveBeenCalledTimes(1); + currentAcl = ['user::rw-', 'group::rw-', 'mask::rw-', 'other::rw-']; lockedCommands.length = 0; await manager.cleanup(); @@ -570,11 +571,11 @@ describe('CloudHypervisorVmmIdentityManager', () => { ] as const)( 'completes two cleanup records after a successful launch restoration: %s', async (_name, cleanupOrder) => { - const baseline = ['user::rw-', 'group::rw-', 'other::---']; const finalBaseline = [ 'user::rw-', 'user:1001:rw-', 'group::rw-', 'mask::rw-', 'other::---', ]; + const baseline = [...finalBaseline]; const identities = [ { uid: 999, gid: 1999 }, { uid: 997, gid: 1997 }, @@ -681,7 +682,7 @@ describe('CloudHypervisorVmmIdentityManager', () => { } expect(durableAclIntents.map((intents) => [...intents])).toEqual([[], []]); - currentAcl = [...finalBaseline]; + currentAcl = ['user::rw-', 'group::rw-', 'mask::rw-', 'other::---']; for (const index of cleanupOrder) await managers[index].cleanup(); expect(currentAcl).toEqual(finalBaseline); diff --git a/src/cloud-hypervisor/vmm-identity.ts b/src/cloud-hypervisor/vmm-identity.ts index 2ef2b776a..eb590844e 100644 --- a/src/cloud-hypervisor/vmm-identity.ts +++ b/src/cloud-hypervisor/vmm-identity.ts @@ -78,6 +78,8 @@ export class CloudHypervisorVmmIdentityManager { private identity: CloudHypervisorVmmIdentity | undefined; private provisionalAccountName: string | undefined; private readonly accessedAclPaths = new Set(); + private readonly accessedAclBaselines = new Map(); + private readonly accessedAclTemporaryUids = new Map(); private readonly aclPaths = new Set(); private readonly aclBaselines = new Map(); private readonly aclTemporaryUids = new Map(); @@ -281,6 +283,10 @@ export class CloudHypervisorVmmIdentityManager { this.dependencies.run, ); await this.observer?.prepareAcl(devicePath, baseline, temporaryUids); + if (!this.accessedAclBaselines.has(devicePath)) { + this.accessedAclBaselines.set(devicePath, baseline); + this.accessedAclTemporaryUids.set(devicePath, temporaryUids); + } await this.dependencies.run(this.tools.setfacl, [ '--modify', `user:${identity.uid}:rw`, devicePath, ]); @@ -330,6 +336,8 @@ export class CloudHypervisorVmmIdentityManager { this.requireIdentity().uid, this.aclTemporaryUids.get(devicePath as CloudHypervisorDevicePath) ?? [], liveTemporaryUids, + this.accessedAclBaselines.get(devicePath as CloudHypervisorDevicePath) ?? baseline, + this.accessedAclTemporaryUids.get(devicePath as CloudHypervisorDevicePath) ?? [], ); } restoredTargets.set(devicePath as CloudHypervisorDevicePath, restoredTarget); @@ -382,6 +390,10 @@ export class CloudHypervisorVmmIdentityManager { this.dependencies.run, ); await this.observer?.prepareAcl(devicePath, baseline, temporaryUids); + if (!this.accessedAclBaselines.has(devicePath)) { + this.accessedAclBaselines.set(devicePath, baseline); + this.accessedAclTemporaryUids.set(devicePath, temporaryUids); + } this.aclPaths.add(devicePath); this.aclBaselines.set(devicePath, baseline); this.aclTemporaryUids.set(devicePath, temporaryUids); From 79f7ed77075981c9702b8ab2e573c0c926e70459 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Wed, 14 Oct 2026 15:54:35 -0700 Subject: [PATCH 2/2] fix: preserve journaled ACL mask Restore the deletion-baseline mask after the last VMM grant is removed. Use the current mask only while a live peer still needs device access. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b641e35-ee0f-404f-b119-2dd2ebe950fa --- src/cloud-hypervisor/device-acl.test.ts | 27 ++++++++++++++++++++++--- src/cloud-hypervisor/device-acl.ts | 20 +++++------------- 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/src/cloud-hypervisor/device-acl.test.ts b/src/cloud-hypervisor/device-acl.test.ts index 90b2615ac..56c560224 100644 --- a/src/cloud-hypervisor/device-acl.test.ts +++ b/src/cloud-hypervisor/device-acl.test.ts @@ -135,10 +135,13 @@ describe('Cloud Hypervisor device ACLs', () => { ]); }); - it('retains the broader historical mask required by a durable runner ACE', () => { + it('restores a durable runner ACE without widening the journaled baseline mask', () => { expect(composeDeviceAcl( ['user::rw-', 'group::r--', 'mask::r--', 'other::---'], - ['user::rw-', 'group::r--', 'mask::r--', 'other::---'], + [ + 'user::rw-', 'user:999:rw-', + 'group::r--', 'mask::rw-', 'other::---', + ], 999, [], [], @@ -149,7 +152,25 @@ describe('Cloud Hypervisor device ACLs', () => { [], )).toEqual([ 'user::rw-', 'user:1001:rw-', - 'group::r--', 'mask::rw-', 'other::---', + 'group::r--', 'mask::r--', 'other::---', + ]); + }); + + it('keeps a runner and mask present only in the journaled deletion baseline', () => { + expect(composeDeviceAcl( + [ + 'user::rw-', 'user:1001:rw-', + 'group::rw-', 'mask::rw-', 'other::---', + ], + ['user::rw-', 'group::rw-', 'other::---'], + 999, + [], + [], + ['user::rw-', 'group::rw-', 'other::---'], + [], + )).toEqual([ + 'user::rw-', 'user:1001:rw-', + 'group::rw-', 'mask::rw-', 'other::---', ]); }); diff --git a/src/cloud-hypervisor/device-acl.ts b/src/cloud-hypervisor/device-acl.ts index 174942c1f..292677766 100644 --- a/src/cloud-hypervisor/device-acl.ts +++ b/src/cloud-hypervisor/device-acl.ts @@ -115,9 +115,11 @@ export function composeDeviceAcl( ]; if (temporaryNamedUsers.size > 0 || durableNamedUsers.size > 0) { - const currentMask = current.find((entry) => entry.startsWith('mask::')); - const accessedMask = accessedBaseline.find((entry) => entry.startsWith('mask::')); - const requiredMask = mergeAclMasks(currentMask, accessedMask); + const baselineMask = baseline.find((entry) => entry.startsWith('mask::')); + const requiredMask = temporaryNamedUsers.size > 0 + ? current.find((entry) => entry.startsWith('mask::'))! + : baselineMask ?? + accessedBaseline.find((entry) => entry.startsWith('mask::'))!; const maskIndex = composed.findIndex((entry) => entry.startsWith('mask::')); if (maskIndex >= 0) composed[maskIndex] = requiredMask; else { @@ -127,15 +129,3 @@ export function composeDeviceAcl( } return parseDeviceAcl(composed.join('\n')); } - -function mergeAclMasks( - currentMask: string | undefined, - accessedMask: string | undefined, -): string { - const permissions = [0, 1, 2].map((index) => { - const current = currentMask?.slice(-3)[index] ?? '-'; - const accessed = accessedMask?.slice(-3)[index] ?? '-'; - return current !== '-' ? current : accessed; - }).join(''); - return `mask::${permissions}`; -}