Update .gitignore, remove obsolete plan doc
This commit is contained in:
parent
5120b1a3b9
commit
e4cdc4beec
2 changed files with 1 additions and 78 deletions
1
.gitignore
vendored
1
.gitignore
vendored
|
|
@ -1,2 +1,3 @@
|
||||||
/target
|
/target
|
||||||
/result
|
/result
|
||||||
|
librust_out.rlib
|
||||||
|
|
|
||||||
|
|
@ -1,78 +0,0 @@
|
||||||
# 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`.
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue