From 35f43057f433673bb29071cd31fb83f7572799b4 Mon Sep 17 00:00:00 2001 From: Daniel Samson <12231216+daniel-samson@users.noreply.github.com> Date: Sun, 9 Aug 2026 09:57:55 +0100 Subject: [PATCH 1/2] kernel: the give paths take the lock, and a give confines afresh after a death MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three holes from the real-AMD audit, one shared root: the delegation flag-day added paths that touch the broker table and the IOMMU records without the big kernel lock, and a loan-return rule whose re-delegation skipped confinement. - device_enumerate walked the table with no lock. The table stopped being a static array in the bounds track — reserve() regrows it through realloc on every boot — so an unlocked reader can be mid-copy out of a slice that device_register on another core has already freed and reused, or pair a fresh count with a stale slice. Each chunk is now snapshotted under the lock; the copy to the user stays outside it. - system_spawn's give ran entirely unlocked — the comment claiming "the lock has not been dropped" was false (spawnProcessSupervised takes and releases it internally). The ownership pre-check now only spares creating a doomed child; the give itself re-checks, confines and moves in one lock hold, and a give that fails after the spawn kills the child rather than leaving it running without the hardware it was spawned for. - A re-delegated device after a driver death was never re-confined. Death tears the domain down before the loan returns to the lender, so the next give found no active record, reassign no-op'd, and the respawned driver ran the device with a V=0 device-table entry and no domain — silently unconfined, the exact fail-open the fail-closed claim was built to remove. Both give paths now share one body (giveDeviceLocked): check first, confine afresh when no record is active — refusing with ECONFINE like the claim — and move last, when nothing can fail. The iommu test drives the death-and-respawn sequence directly; with the old reassign-only behaviour its two confinement checks fail, with this change the suite is 118/118. --- system/kernel/devices-broker.zig | 12 +++- system/kernel/process.zig | 98 ++++++++++++++++++++++++-------- system/kernel/tests.zig | 27 +++++++++ 3 files changed, 112 insertions(+), 25 deletions(-) diff --git a/system/kernel/devices-broker.zig b/system/kernel/devices-broker.zig index 0d6b8a2..2999587 100644 --- a/system/kernel/devices-broker.zig +++ b/system/kernel/devices-broker.zig @@ -477,11 +477,19 @@ pub fn transferErrnoOf(e: TransferError) i64 { /// Note this is deliberately NOT the M13 capability-passing path, which shares a handle /// refcounted — a copy. Exclusivity cannot be expressed that way. pub fn transfer(id: u64, from: u32, to: u32) TransferError!void { + try canTransfer(id, from); + claimed[@intCast(id)] = to; + giver[@intCast(id)] = from; +} + +/// The checks `transfer` will make, without the move. The syscall layer runs them +/// first — under the same lock hold that the transfer itself will run under — so it +/// can refuse, or arrange the IOMMU confinement the move needs, while nothing has +/// mutated yet and there is nothing to roll back. +pub fn canTransfer(id: u64, from: u32) TransferError!void { if (id >= count) return error.NoSuchDevice; const holder = claimed[@intCast(id)] orelse return error.NotHeld; if (holder != from) return error.NotHeld; - claimed[@intCast(id)] = to; - giver[@intCast(id)] = from; } /// The errno a refused `claim` returns to ring 3. (`ECONFINE` — the claim stood but diff --git a/system/kernel/process.zig b/system/kernel/process.zig index e167312..6fa297f 100644 --- a/system/kernel/process.zig +++ b/system/kernel/process.zig @@ -401,7 +401,16 @@ fn systemDeviceEnumerate(state: *architecture.CpuState) void { var copied: u64 = 0; var start: usize = 0; while (copied < cap) { - const filled = devices_broker.enumerateFrom(start, &chunk); + // Snapshot each chunk under the lock: ring-3 device_register grows the table + // with realloc on other cores, so an unlocked read walks a slice that may + // already have been freed — and pairs a fresh `count` with a stale slice. + // The user copy stays outside; the chunk is the kernel's own bytes, and the + // lock windows stay as small as one chunk. + const filled = filled: { + const flags = sync.enter(); + defer sync.leave(flags); + break :filled devices_broker.enumerateFrom(start, &chunk); + }; if (filled == 0) break; start += filled; const take = @min(@as(u64, filled), cap - copied); @@ -409,7 +418,12 @@ fn systemDeviceEnumerate(state: *architecture.CpuState) void { if (!user_memory.copyToUser(t.address_space, buffer_ptr + copied * sz, bytes)) return failErr(state, ipc.EFAULT); copied += take; } - architecture.setSystemCallResult(state, devices_broker.deviceCount()); + const total = total: { + const flags = sync.enter(); + defer sync.leave(flags); + break :total devices_broker.deviceCount(); + }; + architecture.setSystemCallResult(state, total); } /// device_claim(id) -> 0/-errno: take exclusive ownership of a device for this process. @@ -468,21 +482,45 @@ fn systemDeviceTransfer(state: *architecture.CpuState) void { // unreachable for the rest of the boot — no path un-holds a device but task death. if (scheduler.taskByIdLocked(task_id) == null) return failErr(state, ipc.ESRCH); - devices_broker.transfer(device_id, scheduler.current().id, task_id) catch |e| - return failErr(state, devices_broker.transferErrnoOf(e)); + const errno = giveDeviceLocked(device_id, scheduler.current().id, task_id); + if (errno != 0) return failErr(state, errno); + architecture.setSystemCallResult(state, 0); +} - // The device's IOMMU confinement moves with it. The giver confined it when it - // claimed, so the domain exists and the device stays attached — but the record - // still names the giver as owner, which would leave the receiver's DMA buffers - // unbound (every transfer faulting), a giver's death tearing down a domain the - // receiver is using, and the receiver's death leaving one behind. +/// Move a device from `from` to `to` **with its IOMMU confinement** — the shared body +/// of `device_transfer` and spawn's give. Returns 0 or the errno to refuse with. +/// Caller holds the big kernel lock, and has verified the recipient exists. +/// +/// The confinement must move with the device. Usually the giver confined it at claim, +/// so the domain exists, the device stays attached throughout, and `reassign` re-points +/// the record. But after a driver's death the loan came back with the domain torn down +/// (`releaseAllOwnedBy` runs before the broker returns the device to its lender) — so a +/// re-delegation finds no active record, and a bare `reassign` would no-op and hand the +/// device over silently unconfined: V=0 device-table entry, no domain, every later +/// `dma_alloc` bound into nothing. That is the exact fail-open the fail-closed claim +/// exists to remove, so the same rule applies here: confine afresh, and a give that +/// cannot be confined must not stand. +/// +/// Order matters: the checks run first (nothing has mutated, nothing to roll back), +/// the confinement second (its failure refuses cleanly), the move last (it cannot fail +/// once `canTransfer` passed — same lock hold). Public for the in-kernel iommu test, +/// which drives the death-and-respawn sequence against it directly. +pub fn giveDeviceLocked(device_id: u64, from: u32, to: u32) i64 { + devices_broker.canTransfer(device_id, from) catch |e| + return devices_broker.transferErrnoOf(e); + if (devices_broker.pciAddressOf(device_id)) |bdf| { + if (iommu.confinementOwner(device_id) == null and !iommu.confineDevice(device_id, bdf, to)) + return ipc.ECONFINE; + } + devices_broker.transfer(device_id, from, to) catch |e| + return devices_broker.transferErrnoOf(e); // unreachable: checked above under this lock if (devices_broker.pciAddressOf(device_id)) |_| { - iommu.reassign(device_id, task_id); + iommu.reassign(device_id, to); // Bind whatever the receiver has already allocated — the same courtesy the // claim path does for a driver that dma_alloc'd its rings before claiming. - dmaBindOwnerRegionsInto(task_id, device_id); + dmaBindOwnerRegionsInto(to, device_id); } - architecture.setSystemCallResult(state, 0); + return 0; } /// mmio_map(device_id, resource_index) -> virtual_address: map a claimed device's MMIO window into @@ -1079,22 +1117,36 @@ fn systemSpawn(state: *architecture.CpuState) void { // Refuse before creating anything if the device is not the caller's to give — a // spawn that half-succeeds would leave a child running without the hardware it was - // spawned for, which is worse than not spawning it. - if (device_to_give != abi.no_device and devices_broker.ownerOf(device_to_give) != t.id) - return failErr(state, ipc.EPERM); + // spawned for, which is worse than not spawning it. Read under the lock; it drops + // across the spawn, so the give below re-checks under its own hold — this one only + // spares creating a child that was always going to be killed. + if (device_to_give != abi.no_device) { + const flags = sync.enter(); + defer sync.leave(flags); + devices_broker.canTransfer(device_to_give, t.id) catch |e| + return failErr(state, devices_broker.transferErrnoOf(e)); + } const child = spawnProcessSupervised(item.blob, 4, argv[0..argc], t.id, exit_endpoint) catch return fail(state); if (device_to_give != abi.no_device) { - devices_broker.transfer(device_to_give, t.id, child) catch { - // Cannot happen — ownership was checked above and the lock has not been - // dropped — but a spawned child holding nothing is not something to guess - // about, so say so rather than leave it silent. - log.print("/system/kernel: WARNING spawn gave device {d} to task {d} and the transfer failed\n", .{ device_to_give, child }); + // The give runs in one lock hold: check, confine, move (giveDeviceLocked). + // spawnProcessSupervised took and released the lock internally, so the + // pre-check above holds no authority here. The child cannot outrun the + // hand-over — its first device syscall serializes behind this same lock. + const errno = give: { + const flags = sync.enter(); + defer sync.leave(flags); + break :give giveDeviceLocked(device_to_give, t.id, child); }; - if (devices_broker.pciAddressOf(device_to_give)) |_| { - iommu.reassign(device_to_give, child); - dmaBindOwnerRegionsInto(child, device_to_give); + if (errno != 0) { + // The device stopped being the caller's between the pre-check and here, or + // its confinement was refused. A child running without the hardware it was + // spawned for is worse than no child — undo the spawn. The kill's ownership + // sweep also releases anything the give half-did (a fresh confinement dies + // with the child). + _ = killProcess(t.id, child); + return failErr(state, errno); } } architecture.setSystemCallResult(state, child); diff --git a/system/kernel/tests.zig b/system/kernel/tests.zig index 7d03c72..e58a085 100644 --- a/system/kernel/tests.zig +++ b/system/kernel/tests.zig @@ -1491,6 +1491,33 @@ fn iommuTest() void { check("and the previous holder no longer owns it", iommu.confinementOwner(device_id) != me); iommu.releaseAllOwnedBy(me + 1000); check("the new holder's death tears the domain down", iommu.confinementOwner(device_id) == null); + + // The restart hole. A driver's death returns its device to the lender with the + // domain torn down (asserted just above) — so the NEXT delegation of the same + // device finds no active confinement record, and the transfer path's bare + // `reassign` no-ops: the respawned driver would run the device with a V=0 + // device-table entry and no domain, silently unconfined. The give path must + // confine afresh in that case, exactly as a first claim would. + check("the lender holds the returned device", claimOk(device_id, me)); + const driver: u32 = me + 2000; + check("delegating it confines it to the receiver", process.giveDeviceLocked(device_id, me, driver) == 0); + check("the give's confinement names the receiver", iommu.confinementOwner(device_id) == driver); + // The receiver dies: confinement torn down first, then the broker loans the + // device back to the lender — the same order releaseTaskResourcesLocked runs. + iommu.releaseAllOwnedBy(driver); + devices_broker.releaseAllOwnedBy(driver); + check("death returns the loan to the lender", devices_broker.ownerOf(device_id) == me); + check("and leaves the device unconfined", iommu.confinementOwner(device_id) == null); + // The regression this guards: re-delegation after that death. + const respawned: u32 = me + 3000; + check("re-delegation after the death succeeds", process.giveDeviceLocked(device_id, me, respawned) == 0); + check( + "and the respawned driver's device is confined, not silently naked", + iommu.confinementOwner(device_id) == respawned, + ); + iommu.releaseAllOwnedBy(respawned); + devices_broker.releaseAllOwnedBy(respawned); + _ = devices_broker.unclaim(device_id, me); } log("DANOS-IOMMU: enabled base=0x{x} domains active\n", .{pinfo.iommu_base}); From 5cca580066e6a39e5d0868a1e5c771aad0ca833b Mon Sep 17 00:00:00 2001 From: Daniel Samson <12231216+daniel-samson@users.noreply.github.com> Date: Sun, 9 Aug 2026 09:57:55 +0100 Subject: [PATCH 2/2] kernel: AMD-Vi asked for an interrupt where it meant a store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two command encodings checked against the specification: - COMPLETION_WAIT set bit 1 (I, interrupt) instead of bit 0 (S, store), so the IOMMU was never asked to write the sentinel, the poll always exhausted its spins, and completeAndWait returned without any guarantee the preceding invalidation had executed — no invalidation barrier has ever existed, on QEMU or on silicon. The in-code claim that "QEMU's amd-iommu does not implement the store form" was a misdiagnosis of this bug: with S set, QEMU stores the sentinel fine, and the amd-iommu cases now run without the warn line. On real hardware, which fetches commands asynchronously, the missing barrier was an IOTLB use-after-free window: unmap returned before the invalidation was confirmed and the caller freed the frames. - INVALIDATE_IOMMU_PAGES "invalidate everything" used address bits 51:12 all-ones; the architected encoding is bits 62:12 all-ones (the spec's literal 0x7FFF_FFFF_FFFF_F000). Real silicon is free to misread the non-architected form as a bounded range. --- .../kernel/architecture/x86_64/iommu-amd.zig | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/system/kernel/architecture/x86_64/iommu-amd.zig b/system/kernel/architecture/x86_64/iommu-amd.zig index f096905..f3fab5b 100644 --- a/system/kernel/architecture/x86_64/iommu-amd.zig +++ b/system/kernel/architecture/x86_64/iommu-amd.zig @@ -59,8 +59,15 @@ const command_opcode_shift = 60; // opcode in bits 63:60 of qword 0 const command_completion_wait: u64 = 0x01; const command_invalidate_devtab: u64 = 0x02; const command_invalidate_pages: u64 = 0x03; -const completion_wait_store: u64 = 1 << 1; // S: store `data` to the supplied address -const invalidate_pages_all: u64 = 0x000F_FFFF_FFFF_F000 | 1; // address bits 51:12 all-ones + S +// COMPLETION_WAIT qword 0: bit 0 is S (store `data` to the supplied address), bit 1 is +// I (raise an interrupt). An earlier revision set bit 1 and then blamed QEMU for the +// sentinel never landing — with I instead of S the IOMMU is never *asked* to store, on +// QEMU or on silicon, and completeAndWait was no barrier at all. +const completion_wait_store: u64 = 1 << 0; +// INVALIDATE_IOMMU_PAGES address qword, S=1: the architected invalidate-everything +// encoding is bits 62:12 all-ones (the spec's literal 0x7FFF_FFFF_FFFF_F000). The +// previous 51:12 value is a non-architected range real silicon is free to misread. +const invalidate_pages_all: u64 = 0x7FFF_FFFF_FFFF_F000 | 1; const levels: u8 = 4; // 48-bit IOVA, matching the Intel 4-level path @@ -228,11 +235,14 @@ fn submitCommand(qword0: u64, qword1: u64) void { } /// Append a COMPLETION_WAIT (store form) and spin until the IOMMU writes our sentinel to -/// the completion frame. QEMU consumes the command buffer synchronously on the tail- -/// register write, so by the time we poll the prior invalidation is already applied; the -/// store confirmation is belt-and-suspenders for real hardware. If it never lands -/// (QEMU's amd-iommu does not implement the store form), warn ONCE and proceed — the -/// invalidation itself has happened. +/// the completion frame. This is the driver's only ordering barrier: real hardware +/// fetches commands asynchronously, so a preceding invalidation has not happened until +/// this store lands — the callers that free DMA frames after an unmap depend on it. +/// (QEMU consumes the ring synchronously on the tail write and implements the store +/// form fine; the warning below once fired there only because the command carried the +/// I bit instead of S, so no store was ever requested.) If the store never lands, warn +/// ONCE and proceed rather than wedge the claim path — but on real silicon that line +/// means invalidations are unconfirmed and must be treated as a bug report. fn completeAndWait() void { const sentinel: u64 = 0xC0FFEE; ram(completion_frame)[0] = 0; @@ -246,7 +256,7 @@ fn completeAndWait() void { if (spins > 100_000) { if (!completion_warned) { completion_warned = true; - iommu.environment.write("/system/kernel: AMD-Vi COMPLETION_WAIT store not observed — proceeding (QEMU processes commands synchronously)\n"); + iommu.environment.write("/system/kernel: AMD-Vi COMPLETION_WAIT store not observed — proceeding WITHOUT an invalidation barrier\n"); } return; }