Ciao Tommaso,
On Fri, Sep 25, 2026 at 11:23:14AM +0200, Tommaso Califano wrote:
In QEMU’s USB MTP device, usb_mtp_write_metadata() computes
filename_chars = dlen - offsetof(ObjectInfo, filename) where
offsetof(ObjectInfo,filename) = 53. The dlen parameter is the size of the
last USB packet, not the total ObjectInfo size. 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.
Add a guard to return RES_INVALID_OBJECTINFO, preventing the case.
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3992
Signed-off-by: Tommaso Califano <[email protected]>
---
hw/usb/dev-mtp.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 1d8cfd32dc..704e6c533d 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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 (dlen < offsetof(ObjectInfo, filename)) {
+ usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
+ 0, 0, 0, 0);
+ return;
+ }
+
+ filename_chars = dlen - offsetof(ObjectInfo, filename);
/*
* filename is utf-16. We're intentionally doing
--
2.55.0
Thanks for this patch!
I ran checkpatch `./scripts/checkpatch.pl HEAD~1..HEAD` and there is a
style error:
`
ERROR: suspect code indent for conditional statements (4, 9)
#34: FILE: hw/usb/dev-mtp.c:1709:
+ if (dlen < offsetof(ObjectInfo, filename)) {
+ usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
`
Please fix it :)
Also, do we need a Fixes tag?
I think:
Fixes: 90c1a74271ce ("usb-mtp: Limit filename to object information size")
Probably we should also CC QEMU stable.
That said: I'm definitely not a USB expert by any means but I _think_
that we should not use `dlen`, but `d->length`.
Correct me if I'm wrong, but IIUC a transfer can be split across
multiple descriptors, each sized `dlen`. Using `dlen` there, means that we are
checking against the last one. It could happen that dlen is smaller than
d->length, triggering the check.
Does this make sense to you?
Thanks,
Luigi
