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

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

[Tommaso Califano]

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

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

[Luigi Leonardi]

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

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

[Tommaso Califano]

Ciao Luigi,

>
>Thanks for this patch!
>

Thank you for the review!

>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 :)
>

Ops, I am sorry. Solved :)

>Also, do we need a Fixes tag?
>
>I think:
>Fixes: 90c1a74271ce ("usb-mtp: Limit filename to object information size")
>

Probably yes, but I was not aware of its existence. Thanks for pointing
me out.

>Probably we should also CC QEMU stable.
>

I'll cc it in V2.

>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?
>

Yes, you are right. After studying better the code I think the best
solution is to remove dlen and use d->length for computing filename_chars.

Many thanks,
Tommaso