Merge the audit fixes: unlocked give paths, restart re-confinement, AMD-Vi store bit

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;
         }

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

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);

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});