-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
bound cdc-data endpoints against descriptor length in acm_open #3750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
8a2d00f
28daf9e
ea39a8f
eebd0c1
be8e699
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -714,9 +714,21 @@ bool cdch_xfer_cb(uint8_t daddr, uint8_t ep_addr, xfer_result_t event, uint32_t | |
| //--------------------------------------------------------------------+ | ||
| // Enumeration | ||
| //--------------------------------------------------------------------+ | ||
|
|
||
| // Descriptor-walk hardening gated by CFG_TUH_VALIDATION_LEVEL: at NONE the guards collapse to a pass so | ||
| // trusted-device setups pay no code size; at BASIC (default) the walk stays inside the enumeration buffer. | ||
| #if CFG_TUH_VALIDATION_LEVEL >= TUSB_VALIDATION_BASIC | ||
| #define TU_DESC_VALIDATE(_cond) (_cond) | ||
| #else | ||
| #define TU_DESC_VALIDATE(_cond) (true) | ||
| #endif | ||
|
|
||
| static bool open_ep_stream_pair(cdch_interface_t *p_cdc, tusb_desc_endpoint_t const *desc_ep) { | ||
| for (size_t i = 0; i < 2; i++) { | ||
| TU_ASSERT(TUSB_DESC_ENDPOINT == desc_ep->bDescriptorType && TUSB_XFER_BULK == desc_ep->bmAttributes.xfer, 0); | ||
| // pin bLength so tu_desc_next() below cannot walk the second endpoint past a caller-checked bound | ||
| TU_ASSERT(TU_DESC_VALIDATE(sizeof(tusb_desc_endpoint_t) == desc_ep->bLength) && | ||
| TUSB_DESC_ENDPOINT == desc_ep->bDescriptorType && TUSB_XFER_BULK == desc_ep->bmAttributes.xfer, | ||
| 0); | ||
| TU_ASSERT(tuh_edpt_open(p_cdc->daddr, desc_ep)); | ||
| const uint8_t ep_dir = tu_edpt_dir(desc_ep->bEndpointAddress); | ||
| tu_edpt_stream_t *stream = (ep_dir == TUSB_DIR_IN) ? &p_cdc->stream.rx : &p_cdc->stream.tx; | ||
|
|
@@ -1020,8 +1032,12 @@ static uint16_t acm_open(uint8_t daddr, const tusb_desc_interface_t *itf_desc, u | |
| p_desc = tu_desc_next(p_desc); | ||
|
|
||
| // Communication Functional Descriptors | ||
| while ((p_desc < desc_end) && (TUSB_DESC_CS_INTERFACE == tu_desc_type(p_desc))) { | ||
| if (CDC_FUNC_DESC_ABSTRACT_CONTROL_MANAGEMENT == cdc_functional_desc_typeof(p_desc)) { | ||
| // need the 3-byte header (bLength/bDescriptorType/bDescriptorSubType) in bounds before reading it, and a | ||
| // bLength >= 3 both keeps those reads valid and stops a zero-length descriptor from spinning the walk | ||
| while ((p_desc < desc_end) && TU_DESC_VALIDATE(p_desc + 3 <= desc_end) && | ||
| TUSB_DESC_CS_INTERFACE == tu_desc_type(p_desc) && TU_DESC_VALIDATE(tu_desc_len(p_desc) >= 3)) { | ||
| if (CDC_FUNC_DESC_ABSTRACT_CONTROL_MANAGEMENT == cdc_functional_desc_typeof(p_desc) && | ||
| TU_DESC_VALIDATE(p_desc + sizeof(cdc_desc_func_acm_t) <= desc_end)) { | ||
| // save ACM bmCapabilities | ||
| p_cdc->acm.capability = ((cdc_desc_func_acm_t const *) p_desc)->bmCapabilities; | ||
| } | ||
|
|
@@ -1031,23 +1047,29 @@ static uint16_t acm_open(uint8_t daddr, const tusb_desc_interface_t *itf_desc, u | |
|
|
||
| // Open notification endpoint of control interface if any | ||
| if (itf_desc->bNumEndpoints == 1) { | ||
| // whole endpoint descriptor must fit: tuh_edpt_open reads the full struct regardless of bLength | ||
| TU_ASSERT(TU_DESC_VALIDATE(p_desc + sizeof(tusb_desc_endpoint_t) <= desc_end), 0); | ||
| TU_ASSERT(TUSB_DESC_ENDPOINT == tu_desc_type(p_desc), 0); | ||
| const tusb_desc_endpoint_t *desc_ep = (const tusb_desc_endpoint_t *)p_desc; | ||
| TU_ASSERT(tuh_edpt_open(daddr, desc_ep), 0); | ||
|
Comment on lines
+1050
to
1054
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, tu_desc_next there was still advancing by the device bLength, so an inflated value could push p_desc past desc_end before the next check even ran. since we've already asserted the whole 7-byte endpoint fits, i now step forward by sizeof(tusb_desc_endpoint_t) instead, which keeps the pointer inside the window we validated. |
||
| p_cdc->ep_notif = desc_ep->bEndpointAddress; | ||
|
|
||
| p_desc = tu_desc_next(p_desc); | ||
| // advance by the fixed struct size, not device-supplied bLength, so p_desc stays inside the checked window | ||
| p_desc += sizeof(tusb_desc_endpoint_t); | ||
| } | ||
|
|
||
| //------------- Data Interface (if any) -------------// | ||
| if (TUSB_DESC_INTERFACE == tu_desc_type(p_desc)) { | ||
| if (TU_DESC_VALIDATE(p_desc + sizeof(tusb_desc_interface_t) <= desc_end) && | ||
| TUSB_DESC_INTERFACE == tu_desc_type(p_desc)) { | ||
| const tusb_desc_interface_t *data_itf = (const tusb_desc_interface_t *)p_desc; | ||
| if (data_itf->bInterfaceClass == TUSB_CLASS_CDC_DATA) { | ||
| p_desc = tu_desc_next(p_desc); // next to endpoint descriptor | ||
| p_desc += sizeof(tusb_desc_interface_t); // fixed struct size to endpoint descriptor, not device bLength | ||
|
|
||
| // data endpoints expected to be in pairs | ||
| // open_ep_stream_pair consumes exactly two endpoints; require that count and that both fit before reading them | ||
| TU_ASSERT(TU_DESC_VALIDATE(data_itf->bNumEndpoints == 2), 0); | ||
| TU_ASSERT(TU_DESC_VALIDATE(p_desc + 2 * sizeof(tusb_desc_endpoint_t) <= desc_end), 0); | ||
| TU_ASSERT(open_ep_stream_pair(p_cdc, (const tusb_desc_endpoint_t *)p_desc), 0); | ||
| p_desc += data_itf->bNumEndpoints * sizeof(tusb_desc_endpoint_t); | ||
| p_desc += 2 * sizeof(tusb_desc_endpoint_t); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, the functional descriptor walk was still trusting the loop guard, so a short or zero-length CS_INTERFACE descriptor near desc_end could read the subtype past the end (or spin on bLength==0). i've tightened the loop to require the 3-byte header in bounds plus bLength >= 3 before it reads the type/subtype, and the bmCapabilities read now only happens when the whole cdc_desc_func_acm_t fits inside desc_end.