diff mbox series

usb-mtp: Limit filename to object information size

Message ID ab70659d8d5c580bdf150a5f7d5cc60c8e374ffc.1544740018.git.public@hansmi.ch
State New
Headers show
Series usb-mtp: Limit filename to object information size | expand

Commit Message

Michael Hanselmann Dec. 13, 2018, 10:37 p.m. UTC 
The filename length in MTP metadata is specified by the guest. By
trusting it directly it'd theoretically be possible to get the host to
write memory parts outside the filename buffer into a filename. In
practice though there are usually NUL bytes stopping the string
operations.

Also use the opportunity to not assign the filename member twice.

Signed-off-by: Michael Hanselmann <public@hansmi.ch>
---
 hw/usb/dev-mtp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Gerd Hoffmann Dec. 14, 2018, 7:57 a.m. UTC | #1 
On Thu, Dec 13, 2018 at 10:37:06PM +0000, Michael Hanselmann wrote:
> The filename length in MTP metadata is specified by the guest. By
> trusting it directly it'd theoretically be possible to get the host to
> write memory parts outside the filename buffer into a filename. In
> practice though there are usually NUL bytes stopping the string
> operations.
> 
> Also use the opportunity to not assign the filename member twice.

Added to usb patch queue.

thanks,
  Gerd
diff mbox series

Patch

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 100b7171f4..360ca65ee4 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1705,7 +1705,7 @@  free:
     s->write_pending = false;
 }
 
-static void usb_mtp_write_metadata(MTPState *s)
+static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
 {
     MTPData *d = s->data_out;
     ObjectInfo *dataset = (ObjectInfo *)d->data;
@@ -1717,7 +1717,8 @@  static void usb_mtp_write_metadata(MTPState *s)
     assert(!s->write_pending);
     assert(p != NULL);
 
-    filename = utf16_to_str(dataset->length, dataset->filename);
+    filename = utf16_to_str(MIN(dataset->length, dlen - offsetof(ObjectInfo, filename)),
+                            dataset->filename);
 
     if (strchr(filename, '/')) {
         usb_mtp_queue_result(s, RES_PARAMETER_NOT_SUPPORTED, d->trans,
@@ -1733,7 +1734,6 @@  static void usb_mtp_write_metadata(MTPState *s)
     s->dataset.filename = filename;
     s->dataset.format = dataset->format;
     s->dataset.size = dataset->size;
-    s->dataset.filename = filename;
     s->write_pending = true;
 
     if (s->dataset.format == FMT_ASSOCIATION) {
@@ -1802,7 +1802,7 @@  static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         if (d->offset == d->length) {
             /* The operation might have already failed */
             if (!s->result) {
-                usb_mtp_write_metadata(s);
+                usb_mtp_write_metadata(s, dlen);
             }
             usb_mtp_data_free(s->data_out);
             s->data_out = NULL;