usb: a device has as many interfaces as it declares
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.
This commit is contained in:
@@ -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 |
|
| 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 |
|
| 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 |
|
| 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 |
|
| L6 | xHCI: a failed `allocateDevice` stops leaking an enabled slot | not started |
|
||||||
|
|
||||||
**Suite:** 115/115 at the start of the run.
|
**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;
|
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
|
generation-tagged ids change the id encoding, which is ABI). Not an unattended
|
||||||
decision.
|
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
|
### Working rules for the run
|
||||||
|
|
||||||
|
|||||||
@@ -221,10 +221,17 @@ fn intervalFor(speed: u32, b_interval: u8) u32 {
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
// Upper bounds on what one device's active configuration describes. A boot
|
/// bound: endpoints recorded per interface
|
||||||
// keyboard or mouse has one interface with one interrupt endpoint; a flash drive
|
/// decided-by: external
|
||||||
// has one interface with two bulk endpoints. Generous for those.
|
/// protects: the fixed endpoint array inside InterfaceInfo
|
||||||
pub const max_interfaces = 4;
|
/// 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;
|
pub const max_endpoints_per_interface = 4;
|
||||||
|
|
||||||
// The endpoint-descriptor facts a class driver needs to talk to an endpoint: its
|
// The endpoint-descriptor facts a class driver needs to talk to an endpoint: its
|
||||||
@@ -256,7 +263,19 @@ const ConfiguredEndpoint = struct {
|
|||||||
dci: u32 = 0,
|
dci: u32 = 0,
|
||||||
ring: ProducerRing = .{ .region = .{ .virtual = 0, .physical = 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
|
// One addressed USB device behind this controller: its hardware slot, its EP0
|
||||||
// (control) transfer ring, the DMA context + bounce buffer the control pipe uses,
|
// (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),
|
device_descriptor: usb_abi.DeviceDescriptor = std.mem.zeroes(usb_abi.DeviceDescriptor),
|
||||||
configuration_value: u8 = 0,
|
configuration_value: u8 = 0,
|
||||||
interface_count: 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.
|
// Transfer rings configured for this device's interrupt/bulk endpoints.
|
||||||
endpoint_ring_count: u8 = 0,
|
endpoint_ring_count: u8 = 0,
|
||||||
endpoint_rings: [max_configured_endpoints]ConfiguredEndpoint = [_]ConfiguredEndpoint{.{}} ** max_configured_endpoints,
|
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
|
// 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).
|
// by the status-change endpoint (and by an initial sweep in setupHub).
|
||||||
hub_change_mask: u32 = 0,
|
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
|
// 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 {
|
fn abandon(self: *Controller, device: *Device) ?*Device {
|
||||||
_ = self;
|
_ = self;
|
||||||
|
device.freeInterfaces();
|
||||||
device.used = false;
|
device.used = false;
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
@@ -1329,7 +1364,7 @@ pub const Controller = struct {
|
|||||||
// Both numbers, so a truncation can never again be invisible: the block the
|
// 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.
|
// 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 });
|
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.
|
// Select the configuration, moving the device to the configured state.
|
||||||
if (!self.controlTransfer(device, usb_abi.setConfiguration(configuration.configuration_value), &.{}, false)) return false;
|
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
|
// and the endpoints that follow it. Endpoints belong to the most recent
|
||||||
// interface. Unknown descriptor types (HID, class-specific) are skipped by
|
// interface. Unknown descriptor types (HID, class-specific) are skipped by
|
||||||
// their length.
|
// their length.
|
||||||
fn parseConfiguration(device: *Device, blob: []const u8) void {
|
/// Two passes: count the alternate-setting-0 interfaces the block declares,
|
||||||
device.interface_count = 0;
|
/// 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 current: ?*InterfaceInfo = null;
|
||||||
var offset: usize = 0;
|
var offset: usize = 0;
|
||||||
while (offset + 2 <= blob.len) {
|
while (offset + 2 <= blob.len) {
|
||||||
@@ -1351,9 +1408,17 @@ pub const Controller = struct {
|
|||||||
switch (@as(usb_abi.DescriptorType, @enumFromInt(descriptor_type))) {
|
switch (@as(usb_abi.DescriptorType, @enumFromInt(descriptor_type))) {
|
||||||
.interface => if (length >= @sizeOf(usb_abi.InterfaceDescriptor)) {
|
.interface => if (length >= @sizeOf(usb_abi.InterfaceDescriptor)) {
|
||||||
const descriptor = std.mem.bytesToValue(usb_abi.InterfaceDescriptor, blob[offset .. offset + @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) {
|
// An interface we do not record MUST clear `current`, or the
|
||||||
current = null; // ignore alternate settings for now
|
// endpoints that follow it attach to the previous one. The cap
|
||||||
} else if (device.interface_count < max_interfaces) {
|
// 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];
|
const slot = &device.interfaces[device.interface_count];
|
||||||
slot.* = .{
|
slot.* = .{
|
||||||
.number = @intFromEnum(descriptor.interface_number),
|
.number = @intFromEnum(descriptor.interface_number),
|
||||||
@@ -1383,6 +1448,7 @@ pub const Controller = struct {
|
|||||||
}
|
}
|
||||||
offset += length;
|
offset += length;
|
||||||
}
|
}
|
||||||
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- endpoint configuration + interrupt / bulk transfers ---------------
|
// --- endpoint configuration + interrupt / bulk transfers ---------------
|
||||||
|
|||||||
@@ -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:descriptor
|
||||||
system/drivers/usb-xhci-bus/usb-xhci-library.zig:head
|
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: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:max_subscriptions
|
||||||
system/drivers/usb-xhci-bus/usb-xhci-library.zig:port_changes
|
system/drivers/usb-xhci-bus/usb-xhci-library.zig:port_changes
|
||||||
system/drivers/usb-xhci-bus/usb-xhci-library.zig:raw
|
system/drivers/usb-xhci-bus/usb-xhci-library.zig:raw
|
||||||
|
|||||||
Reference in New Issue
Block a user