Re: [PATCH] hw/usb: Fix USB MTP heap OOB read via dlen underflow

[Luigi Leonardi <leonardi@redhat.com>]

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 <tcalifan@redhat.com>
>---
> 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