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