From 6328823ef1d200cae78f75ee2aba6e06f974c882 Mon Sep 17 00:00:00 2001 From: Daniel Samson <12231216+daniel-samson@users.noreply.github.com> Date: Sat, 8 Aug 2026 11:53:01 +0100 Subject: [PATCH] usb: a configuration block is as long as the device says it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The driver read the first 512 bytes of a configuration block into a fixed buffer and parsed those. The block's length is the device's own choice (wTotalLength, a u16), so anything larger was silently cut: interfaces past the cut did not exist as far as the host was concerned, while the SET_CONFIGURATION that follows still configured the device for all of them. A headset is 500-900 bytes, a UVC webcam 1-3 KB, a multifunction printer 600+. Now allocated at the declared length, so the ceiling is the field's u16 — the specification's number rather than one of ours. A block shorter than its own 9-byte header is refused rather than trusted. The bring-up line reports the declared length and the bytes actually read, so a truncation can never again be invisible, and usb-large-descriptor asserts they match with a backreference. That case has an honest limit, recorded in its comment: QEMU cannot produce a block over 512 bytes. The boot keyboard, mouse and stick are 34-44, and the largest device available is usb-audio in multi-channel mode at 211 — which is exactly why the suite never caught this, and why it cannot now reproduce the original trigger. What it does catch is the class: any clamp below the attached device's block fails it, verified by pinning the buffer to 128 and watching "config block 211 bytes, read 128" turn the case red. Suite 115 -> 116. --- docs/bounds-track-plan.md | 2 +- .../drivers/usb-xhci-bus/usb-xhci-library.zig | 25 ++++++++++++++----- test/qemu_test.py | 22 ++++++++++++++++ tools/bounds-allowlist.txt | 1 - 4 files changed, 42 insertions(+), 8 deletions(-) diff --git a/docs/bounds-track-plan.md b/docs/bounds-track-plan.md index 3c387f7..c44794f 100644 --- a/docs/bounds-track-plan.md +++ b/docs/bounds-track-plan.md @@ -15,7 +15,7 @@ next one starts.* | L1 | Reclamation: a dead task's registrations die with its claims | **stopped — the step was wrong; see open question 4** | | 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 | not started | +| 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 | | L6 | xHCI: a failed `allocateDevice` stops leaking an enabled slot | not started | diff --git a/system/drivers/usb-xhci-bus/usb-xhci-library.zig b/system/drivers/usb-xhci-bus/usb-xhci-library.zig index d0cf396..7e3d8c5 100644 --- a/system/drivers/usb-xhci-bus/usb-xhci-library.zig +++ b/system/drivers/usb-xhci-bus/usb-xhci-library.zig @@ -1311,12 +1311,25 @@ pub const Controller = struct { const configuration = std.mem.bytesToValue(usb_abi.ConfigurationDescriptor, &header); device.configuration_value = @intFromEnum(configuration.configuration_value); - // Read the whole block into a local buffer and parse it here (in the bus - // driver) so the parse never has to cross the 256-byte IPC boundary. - var blob: [512]u8 = undefined; - const length = @min(configuration.total_length, blob.len); - if (!self.controlTransfer(device, usb_abi.getDescriptor(.configuration, 0, 0, @intCast(length)), blob[0..length], true)) return false; - parseConfiguration(device, blob[0..length]); + // Read the whole block and parse it here (in the bus driver) so the parse + // never has to cross the 256-byte IPC boundary. Sized by the device's own + // wTotalLength — the ceiling is then the field's u16, which is the USB + // specification's, not ours. + // + // This was a fixed 512 with an `@min` clamp, which silently truncated: a + // composite device routinely exceeds it (a headset is 500-900 bytes, a UVC + // webcam 1-3 KB, a multifunction printer 600+), and the interfaces past the + // cut simply did not exist — while SET_CONFIGURATION below still configured + // the device for all of them. QEMU's boot keyboard, mouse and stick are all + // under 100 bytes, which is why it survived. + if (configuration.total_length < header.len) return false; // shorter than its own header + const blob = memory.allocator().alloc(u8, configuration.total_length) catch return false; + defer memory.allocator().free(blob); + if (!self.controlTransfer(device, usb_abi.getDescriptor(.configuration, 0, 0, configuration.total_length), blob, true)) return false; + // 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); // Select the configuration, moving the device to the configured state. if (!self.controlTransfer(device, usb_abi.setConfiguration(configuration.configuration_value), &.{}, false)) return false; diff --git a/test/qemu_test.py b/test/qemu_test.py index a46907f..3f52528 100644 --- a/test/qemu_test.py +++ b/test/qemu_test.py @@ -674,6 +674,28 @@ CASES = [ r"(?=.*hub slot \d+ port \d+ device:.*0x0627)" r"(?=.*usb-hid-keyboard: ok \(device 3)", "fail": r"DANOS-TEST-RESULT: FAIL"}, + # The driver must never truncate a configuration block. Its size is the DEVICE's + # choice (wTotalLength, a u16); the driver used to read the first 512 bytes into a + # fixed buffer and parse those, so interfaces past the cut did not exist while + # SET_CONFIGURATION still configured the device for all of them. + # + # The backreference is the assertion: declared length and bytes read must match. + # + # Honest limit: QEMU cannot produce a block over 512 bytes. Its boot keyboard, + # mouse and stick are 34-44, and the largest device on offer is usb-audio in + # multi-channel mode at 211 — which is why the suite never saw the original bug, + # and why it cannot now reproduce that exact trigger. What this case does catch is + # the class: any clamp below the attached device's block size fails it, verified + # by pinning the buffer to 128 and watching it go red. A real headset (500-900 + # bytes), UVC webcam (1-3 KB) or multifunction printer trips the 512 itself. + {"name": "usb-large-descriptor", + "build_case": "usb-hid", + "smp": 4, + "timeout": 150, + "qemu_extra": ["-device", "qemu-xhci,id=xhci2", + "-device", "usb-audio,bus=xhci2.0,port=1,multi=on"], + "expect": r"(?s)(?=.*usb-xhci-bus: config block (\d\d\d+) bytes, read \1)", + "fail": r"DANOS-TEST-RESULT: FAIL"}, # USB mass storage end to end: the boot usb-storage device (the FAT32 image, # which has a real 0x55AA boot sector) is enough — the manager spawns # usb-storage, which opens the device, runs the Bulk-Only / SCSI bring-up, diff --git a/tools/bounds-allowlist.txt b/tools/bounds-allowlist.txt index dd38ac1..9797ce2 100644 --- a/tools/bounds-allowlist.txt +++ b/tools/bounds-allowlist.txt @@ -76,7 +76,6 @@ system/drivers/usb-xhci-bus/usb-xhci-bus.zig:maker_buffer system/drivers/usb-xhci-bus/usb-xhci-bus.zig:prev_connected system/drivers/usb-xhci-bus/usb-xhci-bus.zig:product_buffer system/drivers/usb-xhci-bus/usb-xhci-bus.zig:product_buffer -system/drivers/usb-xhci-bus/usb-xhci-library.zig:blob system/drivers/usb-xhci-bus/usb-xhci-library.zig:buffer system/drivers/usb-xhci-bus/usb-xhci-library.zig:bytes system/drivers/usb-xhci-bus/usb-xhci-library.zig:data