dsg/usbip-rs
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
usbip-rs/improvements.md
Davíð Steinn Geirsson 5120b1a3b9 fix: improve reliability with typed error handling and poison recovery 
- Replace string-based USB error classification with ErrorKind matching:
  nusb TransferError is now preserved through io::Error instead of being
  destroyed by format!(). Stall→ConnectionReset→EPIPE, Cancelled→
  Interrupted→ENOENT, Disconnected→ConnectionAborted→ESHUTDOWN.
- Replace fragile string matching in interrupt IN retry loop with direct
  TransferError::Cancelled pattern match.
- Replace 21 production Mutex::lock().unwrap() calls with
  .unwrap_or_else(|e| e.into_inner()) to recover from mutex poisoning
  instead of cascading panics across the server.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-25 00:21:10 +00:00

4.5 KiB
Raw Blame History

USB/IP Rust Implementation — Audit & Improvements

CRITICAL: Security Hardening (untrusted client)

1. Endpoint number not validated 0-15

  • File: lib/src/lib.rs:508
  • Issue: header.ep is a u32 from the wire, cast to u8 at line 525. The kernel validates epnum is 0-15 (stub_rx.c:344). A malicious client can send any value; truncation to u8 is silent.
  • Fix: Reject if header.ep > 15.
  • Status: [x] DONE — Added validation rejecting header.ep > 15 before processing.

2. ISO total_len overflow in host passthrough

  • File: lib/src/host.rs:419-421
  • Issue: num_packets * packet_size can overflow or produce a huge allocation. Should use transfer_buffer_length (already validated) instead of computing from first descriptor's length.
  • Status: [x] DONE — total_len now uses request.transfer_buffer_length as usize.

3. try_read().expect() panic in find_ep

  • File: lib/src/device.rs:238
  • Issue: If a concurrent SET_INTERFACE holds the write lock, this panics the host.
  • Fix: Use .read().await or return an error.
  • Status: [x] DONE — try_read() now returns None/skips on contention instead of panicking. Also fixed to_bytes_with_interfaces().

4. Unbounded in-flight URBs (DoS)

  • File: lib/src/lib.rs:463
  • Issue: A malicious client can flood CMD_SUBMIT without waiting for responses, spawning unbounded tasks and HashMap entries.
  • Fix: Cap concurrent in-flight URBs at a reasonable limit (e.g., 256).
  • Status: [x] DONE — Added MAX_IN_FLIGHT_URBS = 256 constant; rejects with EBUSY when exceeded.

HIGH: USB Spec Conformance & Data Integrity

5. String descriptor bLength overflow

  • File: lib/src/device.rs:521
  • Issue: If a string has >126 UTF-16 code units, bytes.len() as u8 * 2 wraps around, producing a malformed descriptor.
  • Fix: Cap or use u16 arithmetic, truncate string if needed.
  • Status: [x] DONE — Truncate UTF-16 to max 126 code units before computing bLength.

6. No devid validation

  • File: lib/src/lib.rs:505
  • Issue: The kernel checks pdu->base.devid matches the imported device. This implementation ignores devid.
  • Fix: Validate devid matches the imported device.
  • Status: [x] DONE — Compute expected devid from device bus_num/dev_num; reject mismatches in CMD_SUBMIT and CMD_UNLINK.

7. Missing standard control requests (simulated devices)

  • File: lib/src/device.rs:346-778
  • Issue: GetStatus, ClearFeature, SetFeature, GetConfiguration, SetAddress are unhandled for simulated devices.
  • Status: [x] DONE — Added GetConfiguration, GetStatus (device/interface/endpoint), ClearFeature, SetFeature, SetAddress handlers.

8. ISO actual_length sum not validated

  • File: lib/src/lib.rs:551-552
  • Issue: No overflow check on the sum of per-packet actual_length. Should use checked_add and cap at transfer_buffer_length.
  • Status: [x] DONE — Use saturating_add fold capped at transfer_buffer_length.

9. Response data length not capped to transfer_buffer_length

  • File: lib/src/lib.rs:569-578
  • Issue: For IN transfers, response data from the handler is sent as-is without verifying it doesn't exceed transfer_buffer_length.
  • Fix: Truncate response data to transfer_buffer_length.
  • Status: [x] DONE — Truncate urb_resp.data if it exceeds transfer_buffer_length for IN transfers.

MEDIUM: Reliability

10. server() panics on bind failure

  • File: lib/src/lib.rs:773
  • Issue: expect("bind to addr") panics instead of returning Result.
  • Status: [x] DONE — Changed return type to Result<()>, replaced expect with ?.

11. Error status mapping via string matching

  • File: lib/src/lib.rs:584-592
  • Issue: Fragile string-based error classification depends on nusb/rusb message text.
  • Status: [x] DONE — Preserve nusb TransferError through io::Error; classify via ErrorKind instead of string matching.

12. Poisoned mutex panics

  • Files: host.rs, device.rs (multiple locations)
  • Issue: All Mutex::lock().unwrap() calls panic if the mutex is poisoned.
  • Status: [x] DONE — Replaced 21 production .lock().unwrap() with .lock().unwrap_or_else(|e| e.into_inner()).

13. debug_assert for path/bus_id length

  • File: lib/src/device.rs:252-260
  • Issue: debug_assert! compiled out in release; silent truncation if path > 256 bytes.
  • Status: [x] DONE — Replaced debug_assert! + resize with truncate + resize.