daniel/danos
1
0
Fork 0
Code Issues 4 Pull Requests Actions Packages Projects 1 Releases Wiki Activity

kernel: the give paths take the lock, and a give confines afresh after a death

Browse Source 
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.
This commit is contained in:
Daniel Samson
2026-08-09 09:57:55 +01:00
parent 72807c20e4
commit 35f43057f4
3 changed files with 112 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
system/kernel/process.zig
+75 -23
View File
@@ -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);