feat(rdpeusb): Add PDU's#1165
feat(rdpeusb): Add PDU's#1165Plaban Roy (playbahn) wants to merge 1 commit intoDevolutions:masterfrom
Conversation
Plaban Roy (playbahn)
force-pushed
the
1136pdu
branch
from
744e5de to
b74f4c4
Compare
|
Didn't really think about the expected CI failures and whatnot. What are some commands I should run before committing? There's the common ones: $ cargo fmt -p <package-i-worked-on>
$ cargo test -p <package-i-worked-on>Adding pre-commit hooks seem like a good idea. Or maybe a dir in project root containing multiple |
| #[repr(u16)] | ||
| #[non_exhaustive] | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub enum UrbFunction { |
There was a problem hiding this comment.
Note on the copy pasta docs for the 40+ variants: I will reword most of it.
|
Hi! Thanks for your work on this. I'm also a GSoC applicant, and this PR is a really great step forward for our shared goals! Have you considered providing a Also, you might want to consider removing the windows-sys dependency if it's not strictly necessary. |
There was a problem hiding this comment.
Great work!
Let me just recommend reading the README of the ironrdp-pdu project: https://github.com/Devolutions/IronRDP/tree/9a1ac3092ee3eac3e81823349d8e027065f5b8f8/crates/ironrdp-pdu
I should move all of this somewhere else more obvious, but I especially want to recommend the section about resilient parsing for enumeration-like values: https://github.com/Devolutions/IronRDP/tree/9a1ac3092ee3eac3e81823349d8e027065f5b8f8/crates/ironrdp-pdu#enumeration-like-types-should-allow-resilient-parsing
This will also save you from writing the two-way conversion logic.
crates/ironrdp-rdpeusb/Cargo.toml
Outdated
| # just for constants, etc | ||
| windows-sys = { version = "0.61.2", features = ["Win32_Devices_Usb"] } |
There was a problem hiding this comment.
issue: I agree with uchouT (@uchouT) here, I think we should redefine the values locally to reduce the number of dependencies. The main issue is that this will probably not compile at all on Linux or macOS.
There was a problem hiding this comment.
Hi. The project does build on my (Arch) Linux system, I am only importing the constants.
But i do agree that we could drop windows-sys in favour of hardcoding the values. I guess the reason why I'm using a foreign crate is that I/we don't wanna be the "source of truth", and so that weight is beared by Microsoft. But again, even the MS-RDPEUSB's text is not perfect https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeusb/a7ea1b33-80bb-4197-a502-ee62394399c0
Regarding the compilation issue, is there some way we can enforce: dont "compile" code from windows-sys, but constants is okay ?
[EDIT] cc uchouT (@uchouT)
There was a problem hiding this comment.
I don’t think we can control further the compilation, but as it appears you’re not having problems on Linux, so it’s probably not failing the way I was expecting. But then, I don’t think it’s too much a problem to vendor a few constants, because they will NEVER change (that would be an insane compatibility hazard in the Windows ecosystem), and even if they were to change we’re better off vendoring them so we can control the migration on our own terms rather than whatever windows-sys is up to. I don’t dislike the idea itself though: it’s generally good to avoid taking all the burden of defining everything. In that case it seems to be ~50ish constants that you still ended up defining locally anyway:
There was a problem hiding this comment.
Agreed. Redefining.
The enum wrappers could be useful for fuzzing as well, but it’s not a big requirement for merging this PR. We could wait until the state machine is actually implemented too! |
That’s okay, we actually encourage this kind of LSP friendly documentation 👍 |
We have |
There was a problem hiding this comment.
nitpick: I’m not a big fan of "common" (or "enums", "structs", etc) modules as it’s semantically weak. If you can’t think of something better we can roll with this for now though.
There was a problem hiding this comment.
How's this im thinking:
pdu
|- header.rs
|- ts_urb
|- mod.rs
|- header.rs
|- chan_notify.rs
|- exchange_caps.rs
|- dev_sink.rs
|- mod.rs
|- req_complete.rs
|- usb_dev.rs
Which means dropping my hacked-togther string stuff and using ironrdp-str. Going through the crate, I noticed MultiSzString errors out when the cch field is zero:
IronRDP/crates/ironrdp-str/src/multi_sz.rs
Lines 541 to 544 in 9a1ac30
cchHwIds (4 bytes): A 32-bit unsigned integer. This field MUST contain the number of Unicode characters in the HardwareIds field. This field MAY be 0x00000000.
HardwareIds (variable): An array of bytes. A variable-length field that specifies a multisz string representing the hardware IDs of the client-side device. If the value in the cchHwIds field is 0x00000000, the HardwareIds buffer MUST NOT be present.
Same for cchCompatIds and CompatibilityIds. This is only the case for MultiSz.
[EDIT] It would also be helpful adding a link to https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/ab35aee7-1cf7-42dc-ac74-d0d7f4ca64f7 for the cch stuff
Unicode character: Unless otherwise specified, a 16-bit UTF-16 code unit.
I was unsure if cch is really WCHAR, which is not really spelled out in RDPEUSB or RDPEDYC, but in the bigger picture that is RDPBCGR
There was a problem hiding this comment.
Good catch. Can you confirm if there are other places where this is a thing? If that’s exceptional, I think we can interpret that as an "optional MULTI_SZ field" where the 0x00000000 value is not so much a MULTI_SZ thing than a way to convey that the value is not present for this PDU. This would translate into Option<MultiSzString>. A good solution could be to use the peek_u32 method and check for the value 0x00000000 prior to decoding MULTI_SZ. If this is a common pattern, we may imagine a MultiSzString::decode_opt method which returns Option<Self> so you don’t have to rewrite the logic everywhere 🙂
There was a problem hiding this comment.
Also, the updated module hierarchy looks nice!
There was a problem hiding this comment.
[EDIT] TLDR: ironrdp-str does not really need to be modified.
Can you confirm if there are other places where this is a thing?
If this is a common pattern...
Only MultiSz, and its used only in ADD_DEVICE::HardwareIds and ADD_DEVICE::CompatibilityIds, decode_opt is not worth it, we can map "cch_0_error" to Ok(None).
What do you think about: and a method on SharedMsgHEader |
If you think that ensuring a specific subset can be useful, you can consider something similar to RDCleanPathPdu vs RDCleanPath, yes! Also, if you are convinced that for certain enums no extension is possible at all, an enum is fine too, but not without drawbacks (you still have to deal with mapping using big |
There was a problem hiding this comment.
Pull request overview
Introduces a new ironrdp-rdpeusb crate and begins implementing MS-RDPEUSB wire-format PDUs using ironrdp-core’s Encode/Decode traits, covering initial interfaces (common header/types, capability exchange, device sink, channel notification, USB device, request completion).
Changes:
- Add
ironrdp-rdpeusbcrate scaffolding (no_std+alloc) and dependencies (includingwindows-sysfor USB constants). - Implement initial PDU structs with Encode/Decode for multiple RDPEUSB interfaces.
- Add shared/common wire types (header, interface/mask/function IDs, UTF-16/MultiSZ helpers, TS_URB header + function codes).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ironrdp-rdpeusb/Cargo.toml | Adds crate dependencies/features for the new RDPEUSB crate. |
| crates/ironrdp-rdpeusb/src/lib.rs | Sets up no_std crate root and exports pdu module. |
| crates/ironrdp-rdpeusb/src/pdu/mod.rs | Declares the initial RDPEUSB PDU modules. |
| crates/ironrdp-rdpeusb/src/pdu/common/mod.rs | Implements shared header/types plus UTF-16 + MultiSZ helpers. |
| crates/ironrdp-rdpeusb/src/pdu/common/ts_urb.rs | Adds TS_URB header and URB function-code mapping. |
| crates/ironrdp-rdpeusb/src/pdu/exchange_caps.rs | Adds capability exchange request/response PDUs. |
| crates/ironrdp-rdpeusb/src/pdu/dev_sink.rs | Adds device sink PDUs and USB device capabilities structure. |
| crates/ironrdp-rdpeusb/src/pdu/chan_notify.rs | Adds channel notification (CHANNEL_CREATED) PDU. |
| crates/ironrdp-rdpeusb/src/pdu/usb_dev.rs | Adds USB device interface PDUs (cancel + register callback). |
| crates/ironrdp-rdpeusb/src/pdu/req_complete.rs | Adds request completion PDU (IOCONTROL_COMPLETION). |
| Cargo.lock | Records the new crate’s dependencies in the lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if src.read_u32() != Self::MINOR_VER { | ||
| return Err(unsupported_value_err!("MinorVersion", "is not: 0".to_owned())); | ||
| } | ||
| if src.read_u32() != Self::MAJOR_VER { |
There was a problem hiding this comment.
ChannelCreated::decode compares the decoded Capabilities field against MAJOR_VER instead of CAPS, so valid messages can be rejected (and invalid ones accepted). Update the comparison to use Self::CAPS for the third read_u32() check.
| if src.read_u32() != Self::MAJOR_VER { | |
| if src.read_u32() != Self::CAPS { |
| fn size(&self) -> usize { | ||
| const INTERFACE: usize = const { size_of::<Interface>() }; // Along with mask | ||
| const MESSAGE_ID: usize = const { size_of::<MessageId>() }; | ||
|
|
||
| let fn_id = if self.function_id.is_some() { | ||
| size_of_val(&self.function_id) | ||
| } else { | ||
| 0 | ||
| }; | ||
|
|
||
| strict_sum(&[INTERFACE + MESSAGE_ID + fn_id]) | ||
| } |
There was a problem hiding this comment.
SharedMsgHeader::size uses size_of_val(&self.function_id) when function_id is present. This returns the in-memory size of Option<FunctionId> (which is not guaranteed to be 4), but the wire encoding always writes a single u32. If this underestimates, ensure_size! + unchecked writes can panic; if it overestimates, callers may allocate incorrect buffer sizes. Compute this as size_of::<u32>() when Some instead.
| fn size(&self) -> usize { | ||
| let header = self.header.size(); | ||
| const NUM_USB_DEVICE: usize = const { size_of_val(&AddDevice::NUM_USB_DEVICE) }; | ||
| const USB_DEVICE: usize = const { size_of::<Interface>() }; | ||
| let device_instance_id = self.device_instance_id.size(); | ||
| let hardware_ids = self.hardware_ids.size(); | ||
| let compatibility_ids = self.compatibility_ids.size(); | ||
| let container_id = self.container_id.size(); | ||
| const USB_DEVICE_CAPS: usize = const { size_of::<UsbDeviceCaps>() }; | ||
|
|
||
| strict_sum(&[header | ||
| + NUM_USB_DEVICE | ||
| + USB_DEVICE | ||
| + device_instance_id | ||
| + hardware_ids | ||
| + compatibility_ids | ||
| + container_id | ||
| + USB_DEVICE_CAPS]) | ||
| } |
There was a problem hiding this comment.
AddDevice::size uses size_of::<UsbDeviceCaps>(), but UsbDeviceCaps's on-wire size is fixed to 28 bytes (it encodes extra constants like CbSize and HcdCapabilities). size_of::<UsbDeviceCaps>() will reflect the Rust struct layout instead (likely 20 bytes), so ensure_size! can under-allocate and cause panics during encoding. Use self.usb_device_caps.size() (or UsbDeviceCaps::FIXED_PART_SIZE) for the wire size instead.
| // push null if not in input | ||
| if let Some(last) = inner.last() { | ||
| if last.cch != 1 || last.inner == vec![0] { | ||
| inner.push(Utf16Le { cch: 1, inner: vec![0] }); | ||
| cch += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
MultiSZ's FromIterator appends an extra null-terminator even when the last element is already exactly a single null (Utf16Le { cch: 1, inner: vec![0] }), because the condition uses last.inner == vec![0] instead of !=. This will produce an incorrect MultiSZ with a duplicated terminator and a mismatched cch.
| #[repr(u32)] | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub enum Interface { | ||
| /// **Exchange Capabilities** interface (ID: `0x00000000`). Used by both client and server to | ||
| /// exchange capablities for interface manipulation. | ||
| /// | ||
| /// * [MS-RDPEUSB § 2.2.3 Interface Manipulation Exchange Capabilities Interface][1] | ||
| /// | ||
| /// [1]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeusb/6aee4e70-9d3b-49d7-a9b9-3c437cb27c8e | ||
| ExchangeCapabilites = 0x0, | ||
|
|
There was a problem hiding this comment.
The public API has a few misspellings that will be hard to change later without breaking users: Interface::ExchangeCapabilites (and the doc text “capablities”) should be ExchangeCapabilities, and similarly several docs use “alloted”/“ID's”. Since this crate is new, fixing these now will avoid long-term compatibility baggage.
| let usb_bus_iface_ver = match src.read_u32() { | ||
| 0x0 => UsbBusIfaceVer::V0, | ||
| 0x1 => UsbBusIfaceVer::V1, | ||
| 0x3 => UsbBusIfaceVer::V2, | ||
| _ => { | ||
| return Err(unsupported_value_err!( | ||
| "UsbBusInterfaceVersion", | ||
| "is not one of: 0x0, 0x1, 0x2".to_owned() | ||
| )) | ||
| } | ||
| }; |
There was a problem hiding this comment.
UsbDeviceCaps::decode matches UsbBusInterfaceVersion value 0x3 to UsbBusIfaceVer::V2, but the enum defines V2 = 0x2 and the error message lists 0x2 as the valid value. This currently rejects the correct 0x2 value and accepts an unexpected 0x3 value.
| if ch == 0x0 { | ||
| break; | ||
| } else if (0xD800..=0xDC00).contains(&ch) { | ||
| ensure_size!(in: src, size: size_of::<u16>()); | ||
| let low_surrogate = src.read_u16(); | ||
| inner.push(low_surrogate); | ||
| } |
There was a problem hiding this comment.
The surrogate-pair detection range (0xD800..=0xDC00) is incorrect: 0xDC00 is the first low-surrogate code unit, while high surrogates are 0xD800..=0xDBFF. Including 0xDC00 here will treat a low surrogate as a high surrogate and read an extra u16, corrupting decoding (and can overrun on truncated inputs). Use the correct high-surrogate range.
| dst.write_u32(self.req_id); | ||
| dst.write_u32(self.hresult); | ||
| dst.write_u32(self.info); | ||
| dst.write_u32(self.out_buf_bytes); |
There was a problem hiding this comment.
IoctlCompletion stores out_buf_bytes separately from out_buf, but encode writes out_buf_bytes verbatim and then writes the entire out_buf. If these ever diverge, the encoded PDU becomes inconsistent (receiver will read out_buf_bytes bytes, not out_buf.len()). Consider deriving out_buf_bytes from out_buf.len() during encoding (or validating they match and returning an error).
| dst.write_u32(self.out_buf_bytes); | |
| dst.write_u32(self.out_buf.len() as u32); |
There was a problem hiding this comment.
OutputBufferSize (4 bytes): A 32-bit unsigned integer. The size, in bytes, of the OutputBuffer field. The value of this field MUST not exceed the value of OutputBufferSize field from IO_CONTROL message. If the HResult field indicates success, this field and the Information field MUST be equal. If the HResult field is equal to HRESULT_FROM_WIN32(ERROR_INSUFFICIENT_BUFFER) then this field is set to the value of OutputBufferSize from IO_CONTROL message and the Information field MUST indicate the expected size of the OutputBuffer field. For any other case this field MUST be set to 0 and the value of the Information field MUST be ignored.
Let's see how the next version of the MR turns out
| /// [2]: super::exchange_capabilities::RimExchangeCapabilityRequest | ||
| /// [3]: super::exchange_capabilities::RimExchangeCapabilityResponse |
There was a problem hiding this comment.
Several intra-doc links reference non-existent module paths (e.g. super::exchange_capabilities::... and super::device_sink::...), but the modules in this crate are named exchange_caps and dev_sink. If the workspace denies rustdoc::broken_intra_doc_links (or treats warnings as errors), docs will fail to build; update these links to the correct module paths.
| /// [2]: super::exchange_capabilities::RimExchangeCapabilityRequest | |
| /// [3]: super::exchange_capabilities::RimExchangeCapabilityResponse | |
| /// [2]: super::exchange_caps::RimExchangeCapabilityRequest | |
| /// [3]: super::exchange_caps::RimExchangeCapabilityResponse |
| #[doc(alias = "USB_DEVICE_CAPABILITES")] | ||
| pub struct UsbDeviceCaps { | ||
| pub usb_bus_iface_ver: UsbBusIfaceVer, | ||
| pub usbdi_ver: UsbdiVer, | ||
| pub supported_usb_ver: SupportedUsbVer, | ||
| pub device_speed: DeviceSpeed, | ||
| pub no_ack_isoch_write_jitter_buf_size: NoAckIsochWriteJitterBufSizeInMs, | ||
| } | ||
|
|
||
| impl UsbDeviceCaps { | ||
| pub const CB_SIZE: u32 = 28; | ||
|
|
||
| pub const HCD_CAPS: u32 = 0; | ||
|
|
||
| #[expect(clippy::as_conversions)] | ||
| const FIXED_PART_SIZE: usize = Self::CB_SIZE as usize; | ||
| } | ||
|
|
||
| impl Encode for UsbDeviceCaps { | ||
| fn encode(&self, dst: &mut WriteCursor<'_>) -> EncodeResult<()> { | ||
| ensure_fixed_part_size!(in: dst); | ||
|
|
||
| #[expect(clippy::as_conversions)] | ||
| { | ||
| dst.write_u32(Self::CB_SIZE); | ||
| dst.write_u32(self.usb_bus_iface_ver as u32); | ||
| dst.write_u32(self.usbdi_ver as u32); | ||
| dst.write_u32(self.supported_usb_ver as u32); | ||
| dst.write_u32(Self::HCD_CAPS); | ||
| dst.write_u32(self.device_speed as u32); | ||
| } | ||
| dst.write_u32(self.no_ack_isoch_write_jitter_buf_size.0); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn name(&self) -> &'static str { | ||
| "USB_DEVICE_CAPABILITES" | ||
| } | ||
|
|
||
| fn size(&self) -> usize { | ||
| Self::FIXED_PART_SIZE |
There was a problem hiding this comment.
UsbDeviceCaps is exposed as USB_DEVICE_CAPABILITES (missing the second “I”) in both #[doc(alias = ...)] and name(). This makes the protocol name harder to search for and is likely a typo versus the MS-RDPEUSB USB_DEVICE_CAPABILITIES structure name.
Plaban Roy (playbahn)
force-pushed
the
1136pdu
branch
from
b74f4c4 to
f4eafdd
Compare
|
Benoît Cortier (@CBenoit) Current status on the PR: Reading up on USB stuff like pipes, endpoints, interfaces etc (I do not know really anything about USB in a programmatic sense, so you'll have to bear with me). I think understanding (even a bit) about how/what stuff happens during USB actions will help in making a better structured PDU set. [EDIT] Could you also please recommend some reading material? I know there the USB specs but I don't know where to look. [EDIT] Looks like Cbenoit is busy. cc Marc-Andre Lureau (@elmarco) Additionally, the process could be sped up if you let me know if there are anything in the Thanks. |
Yes, indeed the vast majority of TS_URB fields are exactly that kind of pass-through number. The spec's recurring phrase throughout section 2.2.9 is "This value is from the X field in URB_Y", signaling that the client/server do not interpret these fields at all; they proxy them to/from the HCD verbatim. The whole TS_URB layer is essentially a thin wire serialization of Windows kernel URB structures. You can probably treat most of urbdrc as a lossless marshalling layer rather than a full USB semantic interpreter. My rule of thumb would be:
So things like But a lot of per-URB payload can probably stay as plain values/newtypes with constants rather than strict enums/structs, unless the spec explicitly requires validation on our side. That includes things like:
So yes: for many For reading material, I’d suggest this order:
|
|
Hi uchouT (@uchouT) Are you working on/planning to work on anything that's blocked by this PDU MR? In which case I could edit my commit to only contain the things you need and which are more concrete/final than other parts of the MR. Thanks. |
Hi Plaban Roy (@playbahn) Thanks for checking. I'm not currently blocked by this MR.
Please feel free to proceed with your original plan and finalize it as you see fit. Good luck with the PR! |
Populate the ironrdp-rdpeusb crate with USB Redirection Virtual Channel PDU structures: - SharedHeader with component/packet_id fields - Capability exchange (RIM_EXCHANGE_CAPABILITY_REQUEST/RESPONSE) - USB device descriptors and device sink PDUs - Channel notification (created/closed) - IO request completion - TS_URB header wrapping standard USB Request Blocks Foundation work for USB device redirection — no runtime or backend yet. Inspired by upstream PR Devolutions#1165 (playbahn). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
First draft.
Figured reviewing should get started on the things that are more or less complete right now, as the whole PR will be pretty chonky.
ping Benoit Carrier (@Ben-Devolutions) Marc-Andre Lureau (@elmarco)
[EDIT]: Why the "redundant" docs that more or less reword the protocol? I heavily use LSP hover, and getting info in editor is certainly better than changing applications.
[EDIT]: Closes #1136