[PATCH 0/9] hw/usb/dev-mtp: Collection of bug fixes

[PATCH 0/9] hw/usb/dev-mtp: Collection of bug fixes

[Tommaso Califano]

This series is a collection of bug fixes for the MTP USB device
(hw/usb/dev-mtp.c), all tracking the same GitLab work item
(https://gitlab.com/qemu-project/qemu/-/work_items/4529).

Although each patch is independent, all the changes are in a single file and
stem from the same issue, so I decided to group them into one series.

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>

Tommaso Califano (9):
  hw/usb/dev-mtp: Fix recursive delete of directories
  hw/usb/dev-mtp: Reset pending write state on cancel, reset and error
  hw/usb/dev-mtp: Check length of class control request
  hw/usb/dev-mtp: Handle invalid characters in utf16_to_str()
  hw/usb/dev-mtp: Validate container length on first data packet
  hw/usb/dev-mtp: Detect end of SendObject data correctly
  hw/usb/dev-mtp: Zero-initialize command and params in handle_data
  hw/usb/dev-mtp: Fix assert failures in usb_mtp_write_metadata()
  hw/usb/dev-mtp: Stop processing SendObjectInfo after an error

 hw/usb/dev-mtp.c | 99 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 73 insertions(+), 26 deletions(-)

-- 
2.56.0

[PATCH 1/9] hw/usb/dev-mtp: Fix recursive delete of directories

[Tommaso Califano]

usb_mtp_deletefn() only recursed into the grandchildren of the target,
never deleting the files directly inside it. The directory was left
non-empty, so rmdir() on the host always failed, and objects nested more
than two levels deep were silently kept.

Recurse into every child of the target instead. Since the root directory
can now actually become empty, skip the root object itself: rmdir()'ing
it and freeing it would leave s->objects empty.

Cc: qemu-stable@nongnu.org
Fixes: ec6206a68f70 ("usb-mtp: Support delete of mtp objects")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
---
 hw/usb/dev-mtp.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 8507e516ab..bc5a6f0891 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1152,19 +1152,20 @@ enum {
 
 static int usb_mtp_deletefn(MTPState *s, MTPObject *o, uint32_t trans)
 {
-    MTPObject *iter, *iter2, *iter2_next;
+    MTPObject *iter, *iter_next;
     int ret = 0;
 
     /*
      * TODO: Add support for Protection Status
      */
 
-    QLIST_FOREACH(iter, &o->children, list) {
-        if (iter->format == FMT_ASSOCIATION) {
-            QLIST_FOREACH_SAFE(iter2, &iter->children, list, iter2_next) {
-                ret |= usb_mtp_deletefn(s, iter2, trans);
-            }
-        }
+    QLIST_FOREACH_SAFE(iter, &o->children, list, iter_next) {
+        ret |= usb_mtp_deletefn(s, iter, trans);
+    }
+
+    /* Avoid deleting the mtp root */
+    if (o->parent == NULL) {
+        return ret ? ret : DELETE_SUCCESS;
     }
 
     if (o->format == FMT_UNDEFINED_OBJECT) {
-- 
2.56.0

[PATCH 2/9] hw/usb/dev-mtp: Reset pending write state on cancel, reset and error

[Tommaso Califano]

The pending state (s->dataset.filename, s->data_out, s->write_pending)
was only partially released when a transaction was canceled, the session
closed, the device reset, or the write failed. This left dangling
pointers and leaked memory.

Free and clear these fields consistently on all those paths, and route
the early error in usb_mtp_write_data() through the common cleanup.

Cc: qemu-stable@nongnu.org
Fixes: 53735bef108c ("usb-mtp: Advertise SendObjectInfo for write support")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
---
 hw/usb/dev-mtp.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index bc5a6f0891..1c14770a9c 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1236,6 +1236,13 @@ static void usb_mtp_object_delete(MTPState *s, uint32_t handle,
     }
 }
 
+static void usb_mtp_reset_pending_write(MTPState *s) {
+    g_free(s->dataset.filename);
+    s->dataset.filename = NULL;
+    s->write_pending = false;
+    s->dataset.size = 0;
+}
+
 static void usb_mtp_command(MTPState *s, MTPControl *c)
 {
     MTPData *data_in = NULL;
@@ -1282,6 +1289,7 @@ 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;
+        usb_mtp_reset_pending_write(s);
         usb_mtp_file_monitor_cleanup(s);
         usb_mtp_object_free(s, QTAILQ_FIRST(&s->objects));
         assert(QTAILQ_EMPTY(&s->objects));
@@ -1504,6 +1512,7 @@ static void usb_mtp_handle_reset(USBDevice *dev)
     s->data_out = NULL;
     g_free(s->result);
     s->result = NULL;
+    usb_mtp_reset_pending_write(s);
 }
 
 static void usb_mtp_handle_control(USBDevice *dev, USBPacket *p,
@@ -1522,9 +1531,7 @@ static void usb_mtp_handle_control(USBDevice *dev, USBPacket *p,
             usb_mtp_data_free(s->data_in);
             s->data_in = NULL;
             if (s->write_pending) {
-                g_free(s->dataset.filename);
-                s->write_pending = false;
-                s->dataset.size = 0;
+                usb_mtp_reset_pending_write(s);
             }
             usb_mtp_data_free(s->data_out);
             s->data_out = NULL;
@@ -1614,7 +1621,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) {
@@ -1691,10 +1698,10 @@ close:
         close(d->fd);
         d->fd = -1;
     }
-    g_free(s->dataset.filename);
-    s->dataset.size = 0;
+    usb_mtp_data_free(s->data_out);
+    s->data_out = NULL;
+    usb_mtp_reset_pending_write(s);
     g_free(path);
-    s->write_pending = false;
 }
 
 static void usb_mtp_write_metadata(MTPState *s)
@@ -1740,6 +1747,7 @@ static void usb_mtp_write_metadata(MTPState *s)
         next_handle = o->handle;
     }
 
+    g_free(s->dataset.filename);
     s->dataset.filename = filename;
     s->dataset.format = dataset->format;
     s->dataset.size = dataset->size;
-- 
2.56.0

Re: [PATCH 2/9] hw/usb/dev-mtp: Reset pending write state on cancel, reset and error

[marcandre.lureau]

> The pending state (s->dataset.filename, s->data_out, s->write_pending)
> was only partially released when a transaction was canceled, the session
> closed, the device reset, or the write failed. This left dangling
> pointers and leaked memory.
> 
> Free and clear these fields consistently on all those paths, and route
> the early error in usb_mtp_write_data() through the common cleanup.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 53735bef108c ("usb-mtp: Advertise SendObjectInfo for write support")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
> Suggested-by: Hui Peng <https://gitlab.com/benquike>
> Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
> Message-ID: <20261112174907.158825-3-tcalifano@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index bc5a6f089178..1c14770a9c05 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1236,6 +1236,13 @@ static void usb_mtp_object_delete(MTPState *s, uint32_t handle,
>      }
>  }
>  
> +static void usb_mtp_reset_pending_write(MTPState *s) {

QEMU C style puts the opening brace of a function definition on its own line.

> +    g_free(s->dataset.filename);
> +    s->dataset.filename = NULL;
> +    s->write_pending = false;
> +    s->dataset.size = 0;

Consider also clearing dataset.format and dataset.parent_handle here.

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 2/9] hw/usb/dev-mtp: Reset pending write state on cancel, reset and error

[Tommaso Califano]

On Fri Nov 13, 2026 at 9:33 AM CET, marcandre.lureau wrote:
>> The pending state (s->dataset.filename, s->data_out, s->write_pending)
>> was only partially released when a transaction was canceled, the session
>> closed, the device reset, or the write failed. This left dangling
>> pointers and leaked memory.
>> 
>> Free and clear these fields consistently on all those paths, and route
>> the early error in usb_mtp_write_data() through the common cleanup.
>> 
>> Cc: qemu-stable@nongnu.org
>> Fixes: 53735bef108c ("usb-mtp: Advertise SendObjectInfo for write support")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
>> Suggested-by: Hui Peng <https://gitlab.com/benquike>
>> Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
>> Message-ID: <20261112174907.158825-3-tcalifano@redhat.com>
>> ---
>>  hw/usb/dev-mtp.c | 22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
>> index bc5a6f089178..1c14770a9c05 100644
>> --- a/hw/usb/dev-mtp.c
>> +++ b/hw/usb/dev-mtp.c
>> @@ -1236,6 +1236,13 @@ static void usb_mtp_object_delete(MTPState *s, uint32_t handle,
>>      }
>>  }
>>  
>> +static void usb_mtp_reset_pending_write(MTPState *s) {
>
> QEMU C style puts the opening brace of a function definition on its own line.
>

Yes, sorry I miss that.

>> +    g_free(s->dataset.filename);
>> +    s->dataset.filename = NULL;
>> +    s->write_pending = false;
>> +    s->dataset.size = 0;
>
> Consider also clearing dataset.format and dataset.parent_handle here.

I'll check it.

Thanks,
Tommaso

[PATCH 3/9] hw/usb/dev-mtp: Check length of class control request

[Tommaso Califano]

usb_mtp_handle_control() reads a 16-bit event code from the request
data without checking that the host actually sent it. A request with
wLength < 2 makes the device act on stale bytes left in the control
buffer.

STALL requests that are too short to hold the event code.

Cc: qemu-stable@nongnu.org
Fixes: 47bff13cea8d ("dev-mtp: add support for canceling transaction")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
---
 hw/usb/dev-mtp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 1c14770a9c..1a26f681ce 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1525,6 +1525,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;
-- 
2.56.0

Re: [PATCH 3/9] hw/usb/dev-mtp: Check length of class control request

[marcandre.lureau]

On Thu, 12 Nov 2026 18:49:00 +0100, Tommaso Califano <tcalifano@redhat.com> wrote:
> usb_mtp_handle_control() reads a 16-bit event code from the request
> data without checking that the host actually sent it. A request with
> wLength < 2 makes the device act on stale bytes left in the control
> buffer.
> 
> STALL requests that are too short to hold the event code.
> 
> [...]

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

[PATCH 4/9] hw/usb/dev-mtp: Handle invalid characters in utf16_to_str()

[Tommaso Califano]

wcstombs(NULL, wstr, 0) returns (size_t)-1 when the string contains a
character that cannot be converted. The return value was not checked, so
dlen + 1 wrapped to 0, g_malloc(0) returned NULL, and
usb_mtp_write_metadata() dereferenced it in strchr().

Use size_t, return NULL on conversion failure, and reject the
filename in usb_mtp_write_metadata() when that happens.

Cc: qemu-stable@nongnu.org
Fixes: 6de02a13232a ("usb-mtp: fix utf16_to_str")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
---
 hw/usb/dev-mtp.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 1a26f681ce..7233470558 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1563,7 +1563,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++) {
@@ -1572,9 +1573,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;
 }
@@ -1739,7 +1744,7 @@ static void usb_mtp_write_metadata(MTPState *s)
     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);
-- 
2.56.0

Re: [PATCH 4/9] hw/usb/dev-mtp: Handle invalid characters in utf16_to_str()

[marcandre.lureau]

> wcstombs(NULL, wstr, 0) returns (size_t)-1 when the string contains a
> character that cannot be converted. The return value was not checked, so
> dlen + 1 wrapped to 0, g_malloc(0) returned NULL, and
> usb_mtp_write_metadata() dereferenced it in strchr().
> 
> Use size_t, return NULL on conversion failure, and reject the
> filename in usb_mtp_write_metadata() when that happens.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 6de02a13232a ("usb-mtp: fix utf16_to_str")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
> Suggested-by: Hui Peng <https://gitlab.com/benquike>
> Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
> Message-ID: <20261112174907.158825-5-tcalifano@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 1a26f681ce5f..7233470558db 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1563,7 +1563,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++) {
> @@ -1572,9 +1573,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);

Can we replace this function with g_utf16_to_utf8() which is not
sensible to locale. If not, a comment could be useful.

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 4/9] hw/usb/dev-mtp: Handle invalid characters in utf16_to_str()

[Tommaso Califano]

On Fri Nov 13, 2026 at 9:33 AM CET, marcandre.lureau wrote:
>> wcstombs(NULL, wstr, 0) returns (size_t)-1 when the string contains a
>> character that cannot be converted. The return value was not checked, so
>> dlen + 1 wrapped to 0, g_malloc(0) returned NULL, and
>> usb_mtp_write_metadata() dereferenced it in strchr().
>> 
>> Use size_t, return NULL on conversion failure, and reject the
>> filename in usb_mtp_write_metadata() when that happens.
>> 
>> Cc: qemu-stable@nongnu.org
>> Fixes: 6de02a13232a ("usb-mtp: fix utf16_to_str")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
>> Suggested-by: Hui Peng <https://gitlab.com/benquike>
>> Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
>> Message-ID: <20261112174907.158825-5-tcalifano@redhat.com>
>> ---
>>  hw/usb/dev-mtp.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
>> index 1a26f681ce5f..7233470558db 100644
>> --- a/hw/usb/dev-mtp.c
>> +++ b/hw/usb/dev-mtp.c
>> @@ -1563,7 +1563,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++) {
>> @@ -1572,9 +1573,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);
>
> Can we replace this function with g_utf16_to_utf8() which is not
> sensible to locale. If not, a comment could be useful.

Sure. I wasn’t aware of that function, but it does seem like a perfect
fit. Thanks!

Thanks,
Tommaso

[PATCH 5/9] hw/usb/dev-mtp: Validate container length on first data packet

[Tommaso Califano]

usb_mtp_get_data() subtracts sizeof(mtp_container) from the number of
bytes received, without checking. A short packet or a header declaring
less than 12 bytes makes these unsigned values wrap around.

Reject the transfer if is smaller than the container header.

Cc: qemu-stable@nongnu.org
Fixes: 88d5f381ecb2 ("usb-mtp: Introduce write support for MTP objects")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
---
 hw/usb/dev-mtp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 7233470558..6fd92e5a68 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1784,6 +1784,13 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
             return;
     }
     if (d->first) {
+        if (cpu_to_le32(container->length) < 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 */
-- 
2.56.0

Re: [PATCH 5/9] hw/usb/dev-mtp: Validate container length on first data packet

[marcandre.lureau]

> usb_mtp_get_data() subtracts sizeof(mtp_container) from the number of
> bytes received, without checking. A short packet or a header declaring
> less than 12 bytes makes these unsigned values wrap around.
> 
> Reject the transfer if is smaller than the container header.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 88d5f381ecb2 ("usb-mtp: Introduce write support for MTP objects")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
> Suggested-by: Hui Peng <https://gitlab.com/benquike>
> Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
> Message-ID: <20261112174907.158825-6-tcalifano@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 7233470558db..6fd92e5a687b 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1784,6 +1784,13 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
>              return;
>      }
>      if (d->first) {
> +        if (cpu_to_le32(container->length) < 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;

Should it call usb_mtp_reset_pending_write(s) here as well?

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 5/9] hw/usb/dev-mtp: Validate container length on first data packet

[Tommaso Califano]

On Fri Nov 13, 2026 at 9:33 AM CET, marcandre.lureau wrote:
>> usb_mtp_get_data() subtracts sizeof(mtp_container) from the number of
>> bytes received, without checking. A short packet or a header declaring
>> less than 12 bytes makes these unsigned values wrap around.
>> 
>> Reject the transfer if is smaller than the container header.
>> 
>> Cc: qemu-stable@nongnu.org
>> Fixes: 88d5f381ecb2 ("usb-mtp: Introduce write support for MTP objects")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
>> Suggested-by: Hui Peng <https://gitlab.com/benquike>
>> Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
>> Message-ID: <20261112174907.158825-6-tcalifano@redhat.com>
>> ---
>>  hw/usb/dev-mtp.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
>> index 7233470558db..6fd92e5a687b 100644
>> --- a/hw/usb/dev-mtp.c
>> +++ b/hw/usb/dev-mtp.c
>> @@ -1784,6 +1784,13 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
>>              return;
>>      }
>>      if (d->first) {
>> +        if (cpu_to_le32(container->length) < 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;
>
> Should it call usb_mtp_reset_pending_write(s) here as well?

Yes, I'll add it.

Thanks,
Tommaso

[PATCH 6/9] hw/usb/dev-mtp: Detect end of SendObject data correctly

[Tommaso Califano]

The end of a SendObject data phase was detected by a short packet,
assuming a 64-byte max packet size. That only holds for full speed:
on high and super speed endpoints, transfers ending on a 64-byte
boundary were misdetected.

Use the endpoint's max_packet_size. Also end the transfer as soon as
the size declared in ObjectInfo has been received, and fail it with
RES_INCOMPLETE_TRANSFER if a short packet arrives before that.

Cc: qemu-stable@nongnu.org
Fixes: 3e096650a64d ("dev-mtp: Add support for > 4GB file transfers")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
---
 hw/usb/dev-mtp.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 6fd92e5a68..82de3ee401 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1845,7 +1845,27 @@ 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) {
+        /*
+         * The transfer ends on:
+         *  - A short packet
+         *  - A Zero Length Packet
+         *  - When the declared size has been received.
+         * A size of 0xFFFFFFFF means the object is >= 4GiB and its real size
+         * is unknown, so it can be neither used to detect the end nor checked
+         * against the received length.
+         */
+        if ((p->iov.size % p->ep->max_packet_size) || !p->iov.size ||
+            (s->dataset.size != 0xFFFFFFFF && s->dataset.size == d->offset)) {
+            /* Size is known (< 4GiB) but received data doesn't match it */
+            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;
+                usb_mtp_reset_pending_write(s);
+                return;
+            }
             if (d->length == MTP_WRITE_BUF_SZ) {
                 d->write_status = WRITE_END;
             } else {
-- 
2.56.0

[PATCH 7/9] hw/usb/dev-mtp: Zero-initialize command and params in handle_data

[Tommaso Califano]

When a command container carries fewer than 5 parameters,
usb_mtp_handle_data() passes the unset entries of params[] and fields of
cmd to usb_mtp_command() uninitialized.

Zero-initialize both.

Cc: qemu-stable@nongnu.org
Fixes: 840a178c94db ("usb: mtp filesharing")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
---
 hw/usb/dev-mtp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 82de3ee401..f6f9bf52ed 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1890,9 +1890,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;
 
-- 
2.56.0

[PATCH 8/9] hw/usb/dev-mtp: Fix assert failures in usb_mtp_write_metadata()

[Tommaso Califano]

usb_mtp_write_metadata() can be reached with s->write_pending set, after
two successive SendObjectInfo commands, or with a NULL parent object.
Those states hit the existing assertions and abort QEMU.

Return RES_INVALID_OBJECTINFO before the assertions.

Cc: qemu-stable@nongnu.org
Fixes: 53735bef108c ("usb-mtp: Advertise SendObjectInfo for write support")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
---
 hw/usb/dev-mtp.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index f6f9bf52ed..d01b38aeac 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1723,7 +1723,7 @@ static void usb_mtp_write_metadata(MTPState *s)
     uint32_t next_handle = s->next_handle;
     size_t filename_chars;
 
-    if (d->length <= offsetof(ObjectInfo, filename)) {
+    if (d->length <= offsetof(ObjectInfo, filename) || s->write_pending || !p) {
         usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
                              0, 0, 0, 0);
         return;
@@ -1738,9 +1738,6 @@ static void usb_mtp_write_metadata(MTPState *s)
      */
     filename_chars /= 2;
 
-    assert(!s->write_pending);
-    assert(p != NULL);
-
     filename = utf16_to_str(MIN(dataset->length, filename_chars),
                             dataset->filename);
 
-- 
2.56.0

[PATCH 9/9] hw/usb/dev-mtp: Stop processing SendObjectInfo after an error

[Tommaso Califano]

When SendObjectInfo fails its checks, usb_mtp_command() queues an error
response and then falls through. It still allocates s->data_out. The
data phase is then dropped because a result is pending, so s->data_out
is never released.

Return right after queueing the error response.

Cc: qemu-stable@nongnu.org
Fixes: 53735bef108c ("usb-mtp: Advertise SendObjectInfo for write support")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4529
Suggested-by: Hui Peng <https://gitlab.com/benquike>
Signed-off-by: Tommaso Califano <tcalifano@redhat.com>
---
 hw/usb/dev-mtp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index d01b38aeac..82970b6437 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1401,14 +1401,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) {
@@ -1420,9 +1423,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) {
-- 
2.56.0