feat(rdpeusb)!: tolerate unrecognized device-reported USB capability values#1418
feat(rdpeusb)!: tolerate unrecognized device-reported USB capability values#1418clintcan wants to merge 1 commit into
Conversation
|
I think there is a spec/codecs-layer question here. MS-RDPEUSB 2.2.11 defines But FreeRDP currently writes the raw USB device descriptor Relevant FreeRDP evidence:
If the test confirms that Microsoft accepts higher Fwiw, per |
|
Thanks for digging into this — your FreeRDP trace matches exactly what we captured live, and it's the observation that motivated this PR: a real USB 3.2 flash drive ( On the spec text: agreed, 2.2.11 says "MUST be one of" 0x100/0x110/0x200 — but the structure reads as authored in the USB-2 era and never updated ( On the shape: fair point, and the README's dual-representation critique ( |
There was a problem hiding this comment.
Jumping in to express my preference clearly and give a small review.
First, I appreciate the spec-grounded and empirical investigation. Indeed, the spec does not always describe the reality; actually that’s common enough for RDP. FreeVNC typically does not deviate from the spec by mistake (although it may happen), I like to think of it as an accumulation of targeted workarounds based on what is observed in the real world and/or from reverse engineering to some extent. Always good to question, of course, but in this case, it makes sense: we’ve got USB V3 and variants for a while now.
Moving away from the variant Other fallback which can alias a named value, breaking round-trip identity and Eq, is a requirement on my side before we merge this PR (we are hunting down this kind of pattern across the database).
That’s pretty much the biggest issue here, otherwise everything is pretty neat!
Out of scope here, but it may also be worth evaluate things like the hard-fail in UsbDeviceCaps::check_device_speed (cc uchouT (@uchouT)).
It also sounds like the other out of scope actions mentioned in the PR body are worth considering and implementing in follow up PRs.
| /// A real USB 3.2 device reports `SupportedUsbVersion = 0x320`. This used to be | ||
| /// rejected during decode (tearing down the URBDRC channel); it must now decode | ||
| /// and round-trip. | ||
| #[test] | ||
| fn usb3_capabilities_round_trip() { | ||
| let original = caps(SupportedUsbVer::Usb32); | ||
| let encoded = encode_vec(&original).expect("encode should succeed"); | ||
| let decoded: UsbDeviceCaps = decode(&encoded).expect("USB 3.2 capabilities should decode"); | ||
| assert_eq!(decoded.supported_usb_ver, SupportedUsbVer::Usb32); | ||
| assert_eq!(original, decoded); | ||
| } | ||
|
|
||
| /// An unrecognized device-reported capability value is preserved verbatim in | ||
| /// `Other` rather than failing the decode. | ||
| #[test] | ||
| fn unknown_capability_value_decodes_as_other() { | ||
| let original = caps(SupportedUsbVer::Other(0x9999)); | ||
| let encoded = encode_vec(&original).expect("encode should succeed"); | ||
| let decoded: UsbDeviceCaps = decode(&encoded).expect("unknown version should decode as Other"); | ||
| assert_eq!(decoded.supported_usb_ver, SupportedUsbVer::Other(0x9999)); | ||
| assert_eq!(original, decoded); | ||
| } |
There was a problem hiding this comment.
nitpick: These tests assert round-trip, not the wire contract.
Both tests build a UsbDeviceCaps in Rust, encode it with the crate, then decode; so they lean on the crate's own encode to produce the bytes.
The reported bug is a decode failure of bytes a real device sends; a compensating encode bug would pass these. A decode-from-raw-&[u8] test (28 bytes with 0x320 at the SupportedUsbVersion offset) would reproduce the actual failure and also pin the field order/offset independently of encode.
suggestion: Also it may be possible to use rstest for fixture-based testing and deduplicate code. It would be easy to also include some of the other variants although we don’t need to be exhaustive at this point.
Exactly. I haven’t fully reviewed the TS_URB codec yet, so more similar issues may surface once RDPEUSB gets exercised by real integrations (macrdp, qemu-rdp, etc). I’m currently working on the ironrdp-server RDPEUSB integration, and I expect qemu-rdp to eventually run on top of it. So I’m glad clintcan is uncovering these issues before we run into them deeper in qemu-rdp development :) |
…values `UsbDeviceCaps::decode` rejected any value it did not name for the four device-reported capability fields of USB_DEVICE_CAPABILITIES (UsbBusInterfaceVersion, USBDI_Version, SupportedUsbVersion, DeviceIsHighSpeed). A real USB 3.2 device reports SupportedUsbVersion = 0x320, which was not named, so decoding its ADD_DEVICE failed and tore down the URBDRC channel. Model the four fields as newtype structs wrapping the raw `u32` (the `http::StatusCode` shape the crate's README recommends): named associated constants cover the documented values (SupportedUsbVer also names USB 3.0/3.1/3.2), while any other device-reported value is preserved verbatim instead of failing the decode. Unlike an `Other(u32)` enum fallback, a newtype cannot alias a named value, so round-trip identity and `Eq` stay exact. The framing constants (CbSize, HcdCapabilities) stay strict — they validate the PDU layout, not device data. The added tests decode a real 0x320 device's raw 28-byte USB_DEVICE_CAPABILITIES directly (independent of this crate's own encode, and pinning the field offset) and round-trip every named version plus an unnamed value. BREAKING CHANGE: SupportedUsbVer/UsbdiVer/UsbBusIfaceVer/DeviceSpeed are now newtype structs over u32 with associated constants (e.g. SupportedUsbVer::USB_20, DeviceSpeed::HIGH_SPEED) instead of fieldless enums; construct an arbitrary value with `from_u32` and read the wire value with `to_u32`.
clintcan
force-pushed
the
feat/rdpeusb-lenient-caps
branch
from
c38037b to
80c7c8b
Compare
|
Reshaped in
On the spec datapoint: still glad to fold in your Microsoft-server result whenever you get to it, but as discussed it's decoupled from the merge now — the decoder tolerates whatever the device reports either way. The out-of-scope items (the |
3 participants
Problem
UsbDeviceCaps::decode(MS-RDPEUSBUSB_DEVICE_CAPABILITIES, 2.2.11) rejects anyvalue it doesn't explicitly name for the four device-reported capability
fields —
UsbBusInterfaceVersion,USBDI_Version,SupportedUsbVersion,DeviceIsHighSpeed. In particularSupportedUsbVersiononly knew USB 1.0/1.1/2.0(
0x100/0x110/0x200).A real USB 3.2 device reports
SupportedUsbVersion = 0x320, which isn't named,so decoding its
ADD_DEVICEreturnsunsupported_value_err!— and that errorpropagates out of the DVC processor, tearing down the URBDRC channel. A modern
USB-3 device therefore can't be redirected at all.
Fix
Make the four device-reported enums data-carrying: the named values plus an
Other(u32)fallback (withfrom_u32/to_u32helpers replacing the#[repr(u32)]+as u32encode), so an unrecognized value is preserved verbatiminstead of failing the decode.
SupportedUsbVeradditionally names the USB3.0/3.1/3.2 versions (
0x300/0x310/0x320) for readable output.The framing constants (
CbSize,HcdCapabilities) stay strict — theyvalidate the PDU layout, not device data, so an invalid value there remains a
genuine decode error.
Encoding named values is byte-identical (the
to_u32results equal the previous#[repr(u32)]discriminants), so this is transparent on the wire for existingdevices.
Verification
ironrdp-testsuite-core(newtests/rdpeusb/sink.rs): aUSB 3.2 capability set decodes and round-trips; an unrecognized value decodes as
Other.FreeRDP-with-
urbdrcclient:ADD_DEVICEnow fully decodes and the deviceenumerates, where it previously errored out.
Breaking change
SupportedUsbVer/UsbdiVer/UsbBusIfaceVer/DeviceSpeedare no longer#[repr(u32)]fieldless enums — each gains anOther(u32)variant (andSupportedUsbVergainsUsb30/Usb31/Usb32). Downstream code matching theseexhaustively will need a wildcard arm. No other crate in this workspace is
affected.
Deliberately out of scope (happy to fold in if you'd prefer)
to_supported_usb_versionstill mapsbcdUSBto atmost
Usb20; it could emit the new USB-3 versions so an announced devicereports its true
bcdUSB. Kept separate since this PR targets the decode path.NoAckIsochWriteJitterBufferSizeInMsdecode stays strict (0or10..=512perspec) — only the version/speed enums are loosened here.