From 729b40ece7c4e0c513d9184666349afbe831a743 Mon Sep 17 00:00:00 2001 From: Daniel Samson <12231216+daniel-samson@users.noreply.github.com> Date: Sat, 8 Aug 2026 12:05:33 +0100 Subject: [PATCH] usb: a device has as many interfaces as it declares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit max_interfaces was 4. A composite device — a headset, a webcam with audio, a dock, a multifunction printer — routinely has more, and the fifth did not merely go missing. parseConfiguration's cap branch had no `else`, so when the count was reached `current` kept pointing at interface 3 and the fifth interface's endpoint descriptors were appended to interface 3's array. A class driver bound to interface 3 could then be handed an endpoint belonging to something else entirely, and subscribe or bulk-transfer on it. The alternate-setting arm one line above cleared `current` correctly, which is what the cap branch should have done. Interfaces are now counted from the block in a first pass and allocated to exactly that number, so the ceiling is bNumInterfaces' u8 — the USB specification's. The missing `else` is added too, though after this the bug is unreachable by construction: interface_count cannot reach interfaces.len mid-parse when the list was sized from the same walk. max_configured_endpoints was max_interfaces * max_endpoints_per_interface = 16, a derived guess that moved whenever either input moved. It is now 31, which is the xHCI specification's own limit: a Device Context holds a slot context plus at most 31 endpoint contexts, because the Context Entries field addressing them is 5 bits. max_endpoints_per_interface stays at 4 with its reason recorded — the usb-transfer wire protocol reports exactly max_reported_endpoints (4) per interface, so widening it alone would change nothing a class driver sees. Lifting it is a protocol change. No direct test, and that is written down as open question 5 rather than glossed. The parser is pure and wants a host unit test, but usb-xhci-library.zig imports memory, mmio and time so it cannot be a standalone test root, and QEMU offers nothing that reaches the path — the largest device available is usb-audio,multi=on at 2 interfaces and 211 bytes. The alternate-setting path that shares the same `current = null` logic is exercised by that device. Suite 116/116. --- docs/bounds-track-plan.md | 23 ++++- .../drivers/usb-xhci-bus/usb-xhci-library.zig | 90 ++++++++++++++++--- tools/bounds-allowlist.txt | 2 - 3 files changed, 100 insertions(+), 15 deletions(-) diff --git a/docs/bounds-track-plan.md b/docs/bounds-track-plan.md index c44794f..9c69f37 100644 --- a/docs/bounds-track-plan.md +++ b/docs/bounds-track-plan.md @@ -16,7 +16,7 @@ next one starts.* | L2 | Bounds build check + allowlist; declare what we have already touched | **done** — `zig build bounds`, 273 allowlisted, 5 declared | | L3 | xHCI: slot count from `HCSPARAMS1.MaxSlots`, not 8 | **done** — QEMU reports 64; the driver tracked 8 | | L4 | USB: configuration descriptor sized by `wTotalLength`, not 512 | **done** — QEMU tops out at 211 bytes, so the case catches the class, not the original trigger | -| L5 | USB: interfaces from the descriptor, and the misattributed-endpoint bug | not started | +| L5 | USB: interfaces from the descriptor, and the misattributed-endpoint bug | **done** — fix is by construction; no direct test, see open question 5 | | L6 | xHCI: a failed `allocateDevice` stops leaking an enabled slot | not started | **Suite:** 115/115 at the start of the run. @@ -73,6 +73,27 @@ question down instead of inventing an answer. answered first (tombstone-and-reuse aliases stale ids held by another process; generation-tagged ids change the id encoding, which is ABI). Not an unattended decision. +5. **Driver descriptor parsing cannot be host-tested, so L5's correctness fix ships + without a direct test.** The endpoint-misattribution bug lives in + `parseConfiguration`, a pure function over a byte blob — exactly the shape a host + unit test wants, and `usb-storage/scsi.zig` and `usb-hid/hid-report.zig` already do + this. But `usb-xhci-library.zig` imports `memory`, `mmio` and `time`, so it cannot + be a standalone host-test root, and QEMU offers no device that would exercise the + path anyway: the largest available is `usb-audio,multi=on` at 2 interfaces and 211 + bytes, against a cap of 4. + + Three ways out, and picking one is a judgement about house style rather than a + mechanical step: extract the parser to its own file and wire `usb-abi` into a test + module (build-support currently resolves module names only for `userBinary`); + extract it and import `usb-abi` by relative path (against the import-by-name + convention); or accept QEMU-only coverage and say so. + + Mitigating, and the reason this is recorded rather than blocking: after the fix the + bug is unreachable **by construction**, not by the added `else`. Interfaces are now + allocated to exactly the count the descriptor declares, so `interface_count` can + never reach `interfaces.len` mid-parse. The `else` is belt-and-braces for the + 255-interface clamp. The alternate-setting path that shares it *is* exercised — + `usb-audio` has alternate settings, and the `usb-large-descriptor` case walks them. ### Working rules for the run diff --git a/system/drivers/usb-xhci-bus/usb-xhci-library.zig b/system/drivers/usb-xhci-bus/usb-xhci-library.zig index 7e3d8c5..7f3a9dc 100644 --- a/system/drivers/usb-xhci-bus/usb-xhci-library.zig +++ b/system/drivers/usb-xhci-bus/usb-xhci-library.zig @@ -221,10 +221,17 @@ fn intervalFor(speed: u32, b_interval: u8) u32 { }; } -// Upper bounds on what one device's active configuration describes. A boot -// keyboard or mouse has one interface with one interrupt endpoint; a flash drive -// has one interface with two bulk endpoints. Generous for those. -pub const max_interfaces = 4; +/// bound: endpoints recorded per interface +/// decided-by: external +/// protects: the fixed endpoint array inside InterfaceInfo +/// at-limit: refuse — the surplus endpoint is not recorded and the class driver +/// cannot bind it +/// observed-by: the class driver failing to find the endpoint it wants +/// +/// Held at 4 because the usb-transfer wire protocol reports exactly +/// `max_reported_endpoints` (4) endpoints per interface, so widening this alone +/// would change nothing a class driver can see. Lifting it is a protocol change — +/// docs/bounds-track-plan.md, out of scope for the unattended run. pub const max_endpoints_per_interface = 4; // The endpoint-descriptor facts a class driver needs to talk to an endpoint: its @@ -256,7 +263,19 @@ const ConfiguredEndpoint = struct { dci: u32 = 0, ring: ProducerRing = .{ .region = .{ .virtual = 0, .physical = 0 } }, }; -const max_configured_endpoints = max_interfaces * max_endpoints_per_interface; +/// bound: transfer rings configured on one device slot +/// decided-by: hardware +/// protects: the per-device ring table, sized at compile time +/// at-limit: refuse — configureEndpoint returns null and its caller reports it +/// observed-by: the class driver's own failure to bind an endpoint +/// +/// The xHCI specification's number rather than ours: a Device Context carries a slot +/// context plus at most 31 endpoint contexts, because the Context Entries field that +/// addresses them is 5 bits (DCI 1..31, DCI 1 being the default control endpoint). +/// A device physically cannot present more. This was +/// `max_interfaces * max_endpoints_per_interface` = 16, so it moved whenever either +/// of those two guesses moved. +const max_configured_endpoints = 31; // One addressed USB device behind this controller: its hardware slot, its EP0 // (control) transfer ring, the DMA context + bounce buffer the control pipe uses, @@ -277,7 +296,13 @@ pub const Device = struct { device_descriptor: usb_abi.DeviceDescriptor = std.mem.zeroes(usb_abi.DeviceDescriptor), configuration_value: u8 = 0, interface_count: u8 = 0, - interfaces: [max_interfaces]InterfaceInfo = [_]InterfaceInfo{.{}} ** max_interfaces, + /// The interfaces of the active configuration, allocated at enumeration from the + /// count the configuration descriptor actually declares. This was a fixed 4, and + /// the fifth interface of a composite device (a headset, a webcam with audio, a + /// dock, a multifunction printer) did not merely go missing — see + /// parseConfiguration, where its endpoints were appended to interface 3's list. + interfaces: []InterfaceInfo = &.{}, + // Transfer rings configured for this device's interrupt/bulk endpoints. endpoint_ring_count: u8 = 0, endpoint_rings: [max_configured_endpoints]ConfiguredEndpoint = [_]ConfiguredEndpoint{.{}} ** max_configured_endpoints, @@ -296,6 +321,15 @@ pub const Device = struct { // Downstream ports with a pending change to service (bit P = port P), set // by the status-change endpoint (and by an initial sweep in setupHub). hub_change_mask: u32 = 0, + + /// Release the interface list. Safe to call twice, and on a device that never + /// enumerated — re-enumeration and teardown both come through here. + pub fn freeInterfaces(self: *Device) void { + if (self.interfaces.len != 0) memory.allocator().free(self.interfaces); + self.interfaces = &.{}; + self.interface_count = 0; + } + }; // A standing interrupt-IN subscription: the endpoint's ring is kept armed with a @@ -1162,6 +1196,7 @@ pub const Controller = struct { fn abandon(self: *Controller, device: *Device) ?*Device { _ = self; + device.freeInterfaces(); device.used = false; return null; } @@ -1329,7 +1364,7 @@ pub const Controller = struct { // Both numbers, so a truncation can never again be invisible: the block the // device declared, and the bytes actually fetched and parsed. They must match. std.log.info("config block {d} bytes, read {d}", .{ configuration.total_length, blob.len }); - parseConfiguration(device, blob); + if (!parseConfiguration(device, blob)) return false; // Select the configuration, moving the device to the configured state. if (!self.controlTransfer(device, usb_abi.setConfiguration(configuration.configuration_value), &.{}, false)) return false; @@ -1340,8 +1375,30 @@ pub const Controller = struct { // and the endpoints that follow it. Endpoints belong to the most recent // interface. Unknown descriptor types (HID, class-specific) are skipped by // their length. - fn parseConfiguration(device: *Device, blob: []const u8) void { - device.interface_count = 0; + /// Two passes: count the alternate-setting-0 interfaces the block declares, + /// allocate exactly that many, then fill them. False only on an allocation + /// failure. The count cannot exceed 255 — `bNumInterfaces` is a u8, so that is + /// the USB specification's ceiling and not one of ours. + fn parseConfiguration(device: *Device, blob: []const u8) bool { + var declared: usize = 0; + var count_offset: usize = 0; + while (count_offset + 2 <= blob.len) { + const length = blob[count_offset]; + if (length < 2 or count_offset + length > blob.len) break; + if (@as(usb_abi.DescriptorType, @enumFromInt(blob[count_offset + 1])) == .interface and + length >= @sizeOf(usb_abi.InterfaceDescriptor)) + { + const descriptor = std.mem.bytesToValue(usb_abi.InterfaceDescriptor, blob[count_offset .. count_offset + @sizeOf(usb_abi.InterfaceDescriptor)]); + if (@intFromEnum(descriptor.alternate_setting) == 0 and declared < 255) declared += 1; + } + count_offset += length; + } + + device.freeInterfaces(); + if (declared == 0) return true; + device.interfaces = memory.allocator().alloc(InterfaceInfo, declared) catch return false; + for (device.interfaces) |*interface| interface.* = .{}; + var current: ?*InterfaceInfo = null; var offset: usize = 0; while (offset + 2 <= blob.len) { @@ -1351,9 +1408,17 @@ pub const Controller = struct { switch (@as(usb_abi.DescriptorType, @enumFromInt(descriptor_type))) { .interface => if (length >= @sizeOf(usb_abi.InterfaceDescriptor)) { const descriptor = std.mem.bytesToValue(usb_abi.InterfaceDescriptor, blob[offset .. offset + @sizeOf(usb_abi.InterfaceDescriptor)]); - if (@intFromEnum(descriptor.alternate_setting) != 0) { - current = null; // ignore alternate settings for now - } else if (device.interface_count < max_interfaces) { + // An interface we do not record MUST clear `current`, or the + // endpoints that follow it attach to the previous one. The cap + // branch used to have no `else`, so a fifth interface's endpoints + // were appended to interface 3's array and a class driver bound to + // interface 3 could be handed an endpoint belonging to something + // else entirely — silently, with a truthful-looking count logged. + if (@intFromEnum(descriptor.alternate_setting) != 0 or + device.interface_count >= device.interfaces.len) + { + current = null; + } else { const slot = &device.interfaces[device.interface_count]; slot.* = .{ .number = @intFromEnum(descriptor.interface_number), @@ -1383,6 +1448,7 @@ pub const Controller = struct { } offset += length; } + return true; } // --- endpoint configuration + interrupt / bulk transfers --------------- diff --git a/tools/bounds-allowlist.txt b/tools/bounds-allowlist.txt index 9797ce2..3eea7a4 100644 --- a/tools/bounds-allowlist.txt +++ b/tools/bounds-allowlist.txt @@ -82,8 +82,6 @@ system/drivers/usb-xhci-bus/usb-xhci-library.zig:data system/drivers/usb-xhci-bus/usb-xhci-library.zig:descriptor system/drivers/usb-xhci-bus/usb-xhci-library.zig:head system/drivers/usb-xhci-bus/usb-xhci-library.zig:header -system/drivers/usb-xhci-bus/usb-xhci-library.zig:max_endpoints_per_interface -system/drivers/usb-xhci-bus/usb-xhci-library.zig:max_interfaces system/drivers/usb-xhci-bus/usb-xhci-library.zig:max_subscriptions system/drivers/usb-xhci-bus/usb-xhci-library.zig:port_changes system/drivers/usb-xhci-bus/usb-xhci-library.zig:raw