fix(rdpeusb)!: stop deriving pipe MaximumPacketSize from device speed - #1871
Conversation
`interface_information` computed every `TS_USBD_PIPE_INFORMATION`'s `MaximumPacketSize` from the announced device speed through `usb2_max_payload`, and failed the whole selection when that returned `None`. This makes a SuperSpeed device impossible to configure. In a capture of a Windows Server redirecting a USB 3.1 mass-storage device (`bcdUSB` 0x0310, `bMaxPacketSize0` 9, bulk `wMaxPacketSize` 1024 with SuperSpeed endpoint companions), the server sends `MaximumPacketSize` 0 for every pipe. Signed-off-by: uchouT <i@uchout.moe>
There was a problem hiding this comment.
The RDPEUSB fix itself is minimal and correct: TS_USBD_PIPE_INFORMATION.MaximumPacketSize is hardcoded to 0 and the speed parameter is dropped from select_configuration/select_interface/interface_information, which is what actually unblocks SuperSpeed redirection. However, the PR bundles that fix with substantial new SuperSpeed endpoint-semantics API in ironrdp-usb (is_valid_for Super/SuperPlus arms, max_payload, SUPERSPEED_MAX_PACKET_SIZE_0, rewritten endpoint_zero_max_packet_size) plus a new pub standard_feature module and newly-pub descriptor length constants. None of this is called by ironrdp-rdpeusb or ironrdp-server, the crate's only in-tree consumers, and it contradicts ironrdp-usb's README, which states SuperSpeed descriptor semantics are "intentionally deferred until a non-RDPEUSB consumer requires them." This is speculative scope bundled into a bug fix and should be a separate PR.
Protocol analysis: accepted — Independently re-checked both change_mappings: TsUsbdPipeInfo.max_packet_size=0 in usb.rs and the removed device_speed consumer in urbdrc.rs. MS-RDPEUSB 2.2.9.1.3 gives MaximumPacketSize no documented constraint, and USB_DEVICE_CAPABILITIES decode/validation in pdu/sink.rs is untouched by this PR, so both 'conforms' assessments hold. Also confirmed the handoff's uncertainty note that endpoint_zero_max_packet_size/is_valid_for/max_payload are called only from unit tests, never production code; this review escalates that observation into a scope/architecture finding (contradicts the crate's README design principle) rather than treating it as a pure code-quality footnote.
- blocking / low — crates/ironrdp-usb/src/control.rs
standard_feature (and, in descriptor/mod.rs, the four descriptor length constants changed from private to pub) are new public API surface with zero callers anywhere in the workspace. The PR description justifies them only as 'USB values a consumer otherwise restates' — a hypothetical future external consumer, not a need arising from this fix or any code in this repository. Per the project's own scope discipline this speculative public surface should not ride along with a targeted bug fix; it warrants its own PR once a real consumer is identified. - non_blocking / low — crates/ironrdp-usb/src/descriptor/device.rs
DeviceDescriptor::validate()'s doc comment still says endpoint-zero packet size 'depends on the negotiated speed and is therefore validated separately by endpoint_zero_max_packet_size', but this PR removed the speed parameter from endpoint_zero_max_packet_size entirely — the described dependency no longer exists. The comment should be updated to match the new signature/behavior.
e63933b to
744ed02
Compare
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
LGTM, thank you!
6a5d7c2
into
Devolutions:master
interface_informationderivedTS_USBD_PIPE_INFORMATION.MaximumPacketSizefromthe announced device speed. The client opens the pipe and reports the size it
actually established in
TS_USBD_PIPE_INFORMATION_RESULT, so the request fieldonly states a preference, and a Windows server sends zero.
select_configurationand
select_interfacenow send zero and no longer take a speed.USB_DEVICE_CAPABILITIES([MS-RDPEUSB] 2.2.11) expresses only full and high speed,so the derived value was also unreachable for anything faster.
ironrdp-servertracked the announced speed only to feed those two calls, soUsbCapabilitiesand its accessor go with them; nothing in its public API changes.Observed against a Windows server redirecting a USB 3.1 flash drive from FreeRDP:
it sends
MaximumPacketSize0 for both bulk pipes and receives 1024 back for each.