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