## Summary Multiple guest-triggerable crashes (`assert()`), out-of-bounds reads, and memory/FD leaks exist in the USB MTP device (`hw/usb/dev-mtp.c`): 1. **Reachable Assertions & Integer Underflow in `usb_mtp_write_metadata` / `usb_mtp_get_data`**: A guest sending `CMD_SEND_OBJECT_INFO` with `dlen < offsetof(ObjectInfo, filename)` underflows `filename_chars`, and sending repeated `CMD_SEND_OBJECT_INFO` or mismatched `CMD_SEND_OBJECT` lengths hits `assert(!s->write_pending)`, `assert(p != NULL)`, or `assert(s->dataset.size == d->offset)`. 2. **Control Transfer & Result Packet OOB Reads (`usb_mtp_handle_control` / `usb_mtp_handle_data`)**: `ClassInterfaceOutRequest | 0x64` dereferences `uint16_t *event` without verifying `length >= sizeof(*event)`, and `EP_DATA_IN` copies `s->result` without verifying `p->iov.size >= length`. 3. **Invalid UTF-16 `wcstombs` Return Value (`utf16_to_str`)**: If `wcstombs(NULL, wstr, 0)` returns `(size_t)-1` on invalid wide characters, `dlen + 1` wraps to `0`, allocating a 0-byte buffer (`g_malloc(0)`) and writing out of bounds. 4. **Uninitialized `cmd.argv` & Dataset Leaks**: Uninitialized command parameters and leaked `s->dataset.filename` / `s->data_out` on reset, cancel, or error paths. ## Affected Files - `hw/usb/dev-mtp.c` ## Consolidated Commits ```text 63ea15d325 hw/usb/dev-mtp: fix unsigned integer underflow in usb_mtp_write_metadata d6aa9aa3e3 hw/usb: fix guest-triggerable assertions and length underflows in dwc2, mtp, and ccid 0dccbd172e hw/usb: fix dev-mtp UAF/short packet, ccid short packet, dwc2 async state, and uhci UAF 42456bff30 hw: fix usb-mtp, usb-uas, and apple-sound-chip bounds and pointer bugs bbfc1989ff hw/usb: fix heap UAF, companion HCD type confusion, and assert crashes daa6c53372 hw/usb: fix packet state assertions in dwc2, ohci, uas, core, desc, ehci, and mtp cb044325cc hw: address review feedback and completeness fixes across device models 4780f7b550 hw/usb: fix dev-mtp heap leak and metadata length, hcd-uhci td_addr mask and hcd-dwc2 irq state c3f776a322 hw, net: address remaining patch review comments and edge cases 01e921c892 hw/audio, display, usb, ide, sd, ufs: fix UAFs, asserts and RPMB/MTP bugs 9a005a51f3 hw/i3c, cxl, iommu, dma, s390x, vfio: fix OOB, reentrancy and loop bugs ``` ## Proposed Consolidated Patch (`8db19f3ad9..HEAD`) ```diff diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 1d8cfd32dc..fdfa14e1ac 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -1152,19 +1152,19 @@ enum { static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans) { - MTPObject *iter, *iter2; + MTPObject *iter, *next; int ret = 0; /* * TODO: Add support for Protection Status */ - QLIST_FOREACH(iter, &o->children, list) { - if (iter->format == FMT_ASSOCIATION) { - QLIST_FOREACH(iter2, &iter->children, list) { - ret |= usb_mtp_deletefn(s, iter2, trans); - } - } + QLIST_FOREACH_SAFE(iter, &o->children, list, next) { + ret |= usb_mtp_deletefn(s, iter, trans); + } + + if (o->parent == NULL) { + return ret ? ret : DELETE_SUCCESS; } if (o->format == FMT_UNDEFINED_OBJECT) { @@ -1281,6 +1281,9 @@ static void usb_mtp_command(MTPState *s, MTPControl *c) trace_usb_mtp_op_close_session(s->dev.addr); s->session = 0; s->next_handle = 0; + g_free(s->dataset.filename); + s->dataset.filename = NULL; + s->write_pending = false; usb_mtp_file_monitor_cleanup(s); usb_mtp_object_free(s, QTAILQ_FIRST(&s->objects)); assert(QTAILQ_EMPTY(&s->objects)); @@ -1392,14 +1395,17 @@ static void usb_mtp_command(MTPState *s, MTPControl *c) if (!FLAG_SET(s, MTP_FLAG_WRITABLE)) { usb_mtp_queue_result(s, RES_STORE_READ_ONLY, c->trans, 0, 0, 0, 0); + return; } else if (c->argv[0] && (c->argv[0] != QEMU_STORAGE_ID)) { /* First parameter points to storage id or is 0 */ usb_mtp_queue_result(s, RES_STORE_NOT_AVAILABLE, c->trans, 0, 0, 0, 0); + return; } else if (c->argv[1] && !c->argv[0]) { /* If second parameter is specified, first must also be specified */ usb_mtp_queue_result(s, RES_DESTINATION_UNSUPPORTED, c->trans, 0, 0, 0, 0); + return; } else { uint32_t handle = c->argv[1]; if (handle == 0xFFFFFFFF || handle == 0) { @@ -1411,9 +1417,11 @@ static void usb_mtp_command(MTPState *s, MTPControl *c) if (o == NULL) { usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE, c->trans, 0, 0, 0, 0); + return; } else if (o->format != FMT_ASSOCIATION) { usb_mtp_queue_result(s, RES_INVALID_PARENT_OBJECT, c->trans, 0, 0, 0, 0); + return; } } if (o) { @@ -1503,6 +1511,10 @@ static void usb_mtp_handle_reset(USBDevice *dev) s->data_out = NULL; g_free(s->result); s->result = NULL; + g_free(s->dataset.filename); + s->dataset.filename = NULL; + s->dataset.size = 0; + s->write_pending = false; } static void usb_mtp_handle_control(USBDevice *dev, USBPacket *p, @@ -1515,6 +1527,10 @@ static void usb_mtp_handle_control(USBDevice *dev, USBPacket *p, switch (request) { case ClassInterfaceOutRequest | 0x64: + if (length < sizeof(*event)) { + p->status = USB_RET_STALL; + break; + } if (*event == EVT_CANCEL_TRANSACTION) { g_free(s->result); s->result = NULL; @@ -1522,6 +1538,7 @@ static void usb_mtp_handle_control(USBDevice *dev, USBPacket *p, s->data_in = NULL; if (s->write_pending) { g_free(s->dataset.filename); + s->dataset.filename = NULL; s->write_pending = false; s->dataset.size = 0; } @@ -1551,7 +1568,8 @@ static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p) static char *utf16_to_str(uint8_t len, uint8_t *str16) { wchar_t *wstr = g_new0(wchar_t, len + 1); - int count, dlen; + int count; + size_t dlen; char *dest; for (count = 0; count < len; count++) { @@ -1560,9 +1578,13 @@ static char *utf16_to_str(uint8_t len, uint8_t *str16) } wstr[count] = 0; - dlen = wcstombs(NULL, wstr, 0) + 1; - dest = g_malloc(dlen); - wcstombs(dest, wstr, dlen); + dlen = wcstombs(NULL, wstr, 0); + if (dlen == (size_t)-1) { + g_free(wstr); + return NULL; + } + dest = g_malloc(dlen + 1); + wcstombs(dest, wstr, dlen + 1); g_free(wstr); return dest; } @@ -1613,7 +1635,7 @@ static void usb_mtp_write_data(MTPState *s, uint32_t handle) if (!parent || !s->write_pending) { usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans, 0, 0, 0, 0); - return; + goto close; } if (s->dataset.filename) { @@ -1690,7 +1712,10 @@ close: close(d->fd); d->fd = -1; } + usb_mtp_data_free(s->data_out); + s->data_out = NULL; g_free(s->dataset.filename); + s->dataset.filename = NULL; s->dataset.size = 0; g_free(path); s->write_pending = false; @@ -1704,22 +1729,19 @@ static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen) MTPObject *o; MTPObject *p = usb_mtp_object_lookup(s, s->dataset.parent_handle); uint32_t next_handle = s->next_handle; - size_t filename_chars = dlen - offsetof(ObjectInfo, filename); - - /* - * filename is utf-16. We're intentionally doing - * integer division to truncate if malicious guest - * sent an odd number of bytes. - */ - filename_chars /= 2; + size_t filename_chars = 0; - assert(!s->write_pending); - assert(p != NULL); + if (dlen < offsetof(ObjectInfo, filename) || s->write_pending || !p) { + usb_mtp_queue_result(s, RES_INVALID_OBJECT_INFO, d->trans, + 0, 0, 0, 0); + return; + } + filename_chars = (dlen - offsetof(ObjectInfo, filename)) / 2; filename = utf16_to_str(MIN(dataset->length, filename_chars), dataset->filename); - if (strchr(filename, '/')) { + if (!filename || strchr(filename, '/')) { usb_mtp_queue_result(s, RES_PARAMETER_NOT_SUPPORTED, d->trans, 0, 0, 0, 0); g_free(filename); @@ -1731,6 +1753,7 @@ static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen) next_handle = o->handle; } + g_free(s->dataset.filename); s->dataset.filename = filename; s->dataset.format = dataset->format; s->dataset.size = dataset->size; @@ -1758,6 +1781,14 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container, return; } if (d->first) { + if (cpu_to_le32(container->length) < sizeof(mtp_container) || + data_len < sizeof(mtp_container)) { + usb_mtp_queue_result(s, RES_PARAMETER_NOT_SUPPORTED, d->trans, + 0, 0, 0, 0); + usb_mtp_data_free(s->data_out); + s->data_out = NULL; + return; + } /* Total length of incoming data */ total_len = cpu_to_le32(container->length) - sizeof(mtp_container); /* Length of data in this packet */ @@ -1801,7 +1832,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container, if (d->data_offset == d->length) { /* The operation might have already failed */ if (!s->result) { - usb_mtp_write_metadata(s, dlen); + usb_mtp_write_metadata(s, d->data_offset); } usb_mtp_data_free(s->data_out); s->data_out = NULL; @@ -1812,9 +1843,20 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container, usb_packet_copy(p, d->data + d->data_offset, dlen); d->offset += dlen; d->data_offset += dlen; - if ((p->iov.size % 64) || !p->iov.size) { - assert((s->dataset.size == 0xFFFFFFFF) || - (s->dataset.size == d->offset)); + if ((p->iov.size % p->ep->max_packet_size) || !p->iov.size || + (s->dataset.size != 0xFFFFFFFF && d->offset == s->dataset.size)) { + if (s->dataset.size != 0xFFFFFFFF && + s->dataset.size != d->offset) { + usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans, + 0, 0, 0, 0); + usb_mtp_data_free(s->data_out); + s->data_out = NULL; + g_free(s->dataset.filename); + s->dataset.filename = NULL; + s->dataset.size = 0; + s->write_pending = false; + return; + } if (d->length == MTP_WRITE_BUF_SZ) { d->write_status = WRITE_END; @@ -1840,9 +1882,9 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container, static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p) { MTPState *s = USB_MTP(dev); - MTPControl cmd; + MTPControl cmd = { 0 }; mtp_container container; - uint32_t params[5]; + uint32_t params[5] = { 0 }; uint16_t container_type; int i, rc; @@ -1905,6 +1947,11 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p) } else if (s->result != NULL) { MTPControl *r = s->result; int length = sizeof(container) + r->argc * sizeof(uint32_t); + if (p->iov.size < length) { + trace_usb_mtp_stall(s->dev.addr, "packet too small"); + p->status = USB_RET_STALL; + return; + } if (r->code == RES_OK) { trace_usb_mtp_success(s->dev.addr, r->trans, (r->argc > 0) ? r->argv[0] : 0, ```