usb_mtp_write_metadata() computed the number of filename characters as
filename_chars = dlen - offsetof(ObjectInfo, filename)
where dlen is the amount of data copied by the last USB packet of the
transfer, not the size of the ObjectInfo dataset. If the final packet has
fewer than 53 bytes, filename_chars underflows to SIZE_MAX, causing
utf16_to_str() to read far beyond the heap buffer, leaking QEMU host
process memory.
Use d->length instead: it is the size of the buffer holding the dataset,
and usb_mtp_write_metadata() is only reached once all d->length bytes
have been received.
d->length is guest-controlled, so reject datasets too small to hold the
fixed part of ObjectInfo before dereferencing it. The check is "<="
rather than "<" because a dataset of exactly offsetof(ObjectInfo,filename)
bytes would yield an empty filename.
Fixes: 90c1a74271ce ("usb-mtp: Limit filename to object information size")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3992
Signed-off-by: Tommaso Califano <[email protected]>
---
V3:
- Restore guard due to guest-controlled d->length.
- Improve commit text for better readability.
v2:
- Remove the dlen guard and variable in favor of using d->length.
hw/usb/dev-mtp.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 1d8cfd32dc..45641c3489 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1696,7 +1696,7 @@ close:
s->write_pending = false;
}
-static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
+static void usb_mtp_write_metadata(MTPState *s)
{
MTPData *d = s->data_out;
ObjectInfo *dataset = (ObjectInfo *)d->data;
@@ -1704,7 +1704,15 @@ 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);
+ size_t filename_chars;
+
+ if (d->length <= offsetof(ObjectInfo, filename)) {
+ usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
+ 0, 0, 0, 0);
+ return;
+ }
+
+ filename_chars = d->length - offsetof(ObjectInfo, filename);
/*
* filename is utf-16. We're intentionally doing
@@ -1801,7 +1809,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);
}
usb_mtp_data_free(s->data_out);
s->data_out = NULL;
--
2.55.0
