device: clamp EP0 OUT data copy to the control transfer buffer#3705
Merged
Conversation
usbd_control_xfer_cb() copied xferred_bytes from the EP0 bounce buffer into the requester's buffer with no bound. A non-compliant host that sends an OUT data packet larger than the control transfer's data_len (= min(len, wLength), the buffer capacity) would overflow that buffer and over-count total_xferred. Clamp xferred_bytes to the remaining buffer space before the memcpy and accounting. This is in the shared control handler, so it hardens every DCD. Found while code-reviewing the MUSB EP0 path in #3699. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens TinyUSB’s shared device control transfer handler (usbd_control_xfer_cb) against EP0 OUT data-stage buffer overflows by bounding how many received bytes are copied into the requester’s buffer. This mitigates malformed/non-compliant host behavior (or buggy DCD reporting) that could otherwise cause memory corruption across all device controllers.
Changes:
- Clamp
xferred_bytesfor EP0 OUT to the remaining requester buffer space (data_len - total_xferred) before copying and accounting. - Prevent out-of-bounds
memcpy()into the caller-provided control transfer buffer when a host sends an oversized OUT packet.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in size
Changes <1% in size
No changes
|
Hardware-in-the-loop (HIL) Test Reporthfp-iar
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run hfp.json
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run tinyusb.json
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run |
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| xmc4500_relax/hid_boot_interface | 13,732 → 13,796 (+64) | — | — | — | 27,516 → 27,644 (+128) | +0.5% |
| xmc4500_relax/cdc_dual_ports | 14,908 → 14,972 (+64) | — | — | — | 29,868 → 29,996 (+128) | +0.4% |
| sipeed_longan_nano/hid_multiple_interface | 14,562 → 14,626 (+64) | — | — | — | 15,868 → 15,932 (+64) | +0.4% |
| sipeed_longan_nano/hid_boot_interface | 14,626 → 14,690 (+64) | — | — | — | 15,884 → 15,948 (+64) | +0.4% |
| xmc4500_relax/video_capture | 15,988 → 16,052 (+64) | — | — | — | 32,056 → 32,184 (+128) | +0.4% |
| xmc4500_relax/uac2_headset | 16,344 → 16,408 (+64) | — | — | — | 32,744 → 32,872 (+128) | +0.4% |
| sipeed_longan_nano/hid_generic_inout | 13,846 → 13,908 (+62) | 466 → 462 (-4) | — | — | 14,980 → 15,038 (+58) | +0.4% |
| xmc4500_relax/uac2_speaker_fb | 17,100 → 17,164 (+64) | — | — | — | 34,256 → 34,384 (+128) | +0.4% |
| sipeed_longan_nano/printer_to_cdc | 16,224 → 16,288 (+64) | — | — | — | 17,458 → 17,522 (+64) | +0.4% |
| sipeed_longan_nano/webusb_serial | 16,412 → 16,476 (+64) | — | — | — | 17,950 → 18,014 (+64) | +0.4% |
Collapse the hand-rolled remain/if clamp in usbd_control_xfer_cb() into tu_min32(), matching the tu_min16 idiom data_stage_xact() already uses for the same "clamp to remaining EP0 capacity" computation, and shorten the comment to the non-obvious why. No behavior change.
Add test_usbd_control_out_overrun_clamp covering the host-overrun case: a vendor OUT request offering an 8-byte buffer while the DCD reports a full max-size packet must complete via the IN status stage instead of re-arming an OUT data packet. Also fix the stale TEST_SOURCE_FILE("usbd_control.c") reference (merged into usbd.c) to keep the control path compiled from source.
ceedling test:all (61 passed); cdc_msc builds for stm32f407disco.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
usbd_control_xfer_cb()copies the bytes a DCD reports for an EP0 OUT data packet straight into the requester's buffer with no bound:ctrl_xfer->bufferis the caller's buffer with capacitydata_len = min(len, wLength). A non-compliant or hostile host that sends an OUT data packet larger thandata_len(e.g. a 64-byte packet answering aSET_LINE_CODINGwhose buffer is 7 bytes) overflows that buffer and over-countstotal_xferred. The per-packet length is bounded by the EP0 max packet size, so the bounce buffer (_ctrl_epbuf.buf,CFG_TUD_ENDPOINT0_BUFSIZE) is safe — the overflow is the copy into the smaller requester buffer.This lives in the shared control handler, so it affects every DCD (surfaced while code-reviewing the MUSB EP0 path in #3699).
Fix
Clamp
xferred_bytesto the remaining buffer space (data_len - total_xferred) before the copy and accounting. No-op for conforming hosts (they never send more thanwLength).Verification
pre-commit run --all-filesclean (format, codespell, Ceedling unit tests incl.test_usbd.c).device/cdc_mscforstm32f407disco.🤖 Generated with Claude Code