Skip to content

fix(rdpeusb)!: stop deriving pipe MaximumPacketSize from device speed - #1871

Merged
Benoît Cortier (CBenoit) merged 1 commit into
Devolutions:masterfrom
uchouT:usb3
Sep 3, 2026
Merged

fix(rdpeusb)!: stop deriving pipe MaximumPacketSize from device speed#1871
Benoît Cortier (CBenoit) merged 1 commit into
Devolutions:masterfrom
uchouT:usb3

Conversation

@uchouT

@uchouT uchouT (uchouT) commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

interface_information derived TS_USBD_PIPE_INFORMATION.MaximumPacketSize from
the announced device speed. The client opens the pipe and reports the size it
actually established in TS_USBD_PIPE_INFORMATION_RESULT, so the request field
only states a preference, and a Windows server sends zero. select_configuration
and select_interface now 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-server tracked the announced speed only to feed those two calls, so
UsbCapabilities and 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 MaximumPacketSize 0 for both bulk pipes and receives 1024 back for each.

`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>
Copilot AI balanced review requested due to automatic review settings September 1, 2026 17:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@uchouT uchouT (uchouT) changed the title fix(usb)!: support SuperSpeed devices fix(rdpeusb)!: support SuperSpeed devices Sep 1, 2026
@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure labels Sep 1, 2026
@uchouT uchouT (uchouT) changed the title fix(rdpeusb)!: support SuperSpeed devices fix(usb)!: support SuperSpeed devices Sep 1, 2026
@github-actions github-actions Bot added scope/cross-cutting Spans multiple architectural boundaries scope/tooling Build, CI, release, or developer tooling ai-reviewed/1 One automated review completed and removed maintainer-required Maintainer review or intervention is required labels Sep 1, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.
  2. 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.

Comment thread crates/ironrdp-usb/src/endpoint.rs Outdated
@uchouT uchouT (uchouT) changed the title fix(usb)!: support SuperSpeed devices fix(rdpeusb)!: stop deriving pipe MaximumPacketSize from device speed Sep 1, 2026
@github-actions github-actions Bot added maintainer-required Maintainer review or intervention is required risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure and removed scope/tooling Build, CI, release, or developer tooling size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure risk/medium Behavioral change that does not substantially alter a core public API labels Sep 1, 2026

@CBenoit Benoît Cortier (CBenoit) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@CBenoit
Benoît Cortier (CBenoit) merged commit 6a5d7c2 into Devolutions:master Sep 3, 2026
54 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/1 One automated review completed breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny scope/core Touches the core architectural tier scope/cross-cutting Spans multiple architectural boundaries size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure

Development

Successfully merging this pull request may close these issues.

3 participants