Skip to contentSkip to conversation

virtio-serial: Trivial fixes, don't copy buffers to host

PATCH Superseded Amit Shah 1 participant 5 messages Dec 10, 2010

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Amit Shah

author
#0 lore raw

v2 cover virtio-serial: Trivial fixes, don't copy buffers to host

Hi,

This patch series converts virtio-serial-bus to use the guest buffers instead of copying over guest data to the host, as suggested by Paul.

In addition, there are some trivial fixes for the virtio-console and virtio-serial code.

v2:
- drop the erroring out patch till we decide what's to be done
- remove goto usage.

Amit Shah (4):
virtio-console: Factor out common init between console and generic
ports
virtio-console: Remove unnecessary braces
virtio-serial: Simplify condition for a while loop
virtio-serial: Don't copy over guest buffer to host

 hw/virtio-console.c    |   34 +++++++++++++++-------------------
 hw/virtio-serial-bus.c |   21 ++++++++++++---------
 2 files changed, 27 insertions(+), 28 deletions(-)
Signature · 1 line
1.7.3.2
author
#1 lore raw

PATCH v2 1/4 virtio-console: Factor out common init between console and generic ports

The initialisation for generic ports and console ports is similar. Factor out the parts that are the same in a different function that can be called from each of the initfns.

Signed-off-by Amit Shah <[email protected]>
---
 hw/virtio-console.c |   31 ++++++++++++++-----------------
 1 files changed, 14 insertions(+), 17 deletions(-)
hw/virtio-console.c +14 -17
index caea11f..d7fe68b 100644
@@ -58,24 +58,28 @@ static void chr_event(void *opaque, int event)
     }
 }
 
-/* Virtio Console Ports */
-static int virtconsole_initfn(VirtIOSerialDevice *dev)
+static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
 {
-    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
-
-    port->info = dev->info;
-
-    port->is_console = true;
+    vcon->port.info = dev->info;
 
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
-        port->info->have_data = flush_buf;
+        vcon->port.info->have_data = flush_buf;
     }
     return 0;
 }
 
+/* Virtio Console Ports */
+static int virtconsole_initfn(VirtIOSerialDevice *dev)
+{
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    port->is_console = true;
+    return generic_port_init(vcon, dev);
+}
+
 static int virtconsole_exitfn(VirtIOSerialDevice *dev)
 {
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);

@@ -115,14 +119,7 @@ static int virtserialport_initfn(VirtIOSerialDevice *dev)
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    port->info = dev->info;
-
-    if (vcon->chr) {
-        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
-                              vcon);
-        port->info->have_data = flush_buf;
-    }
-    return 0;
+    return generic_port_init(vcon, dev);
 }
 
 static VirtIOSerialPortInfo virtserialport_info = {
Signature · 1 line
1.7.3.2
author
#2 lore raw

PATCH v2 2/4 virtio-console: Remove unnecessary braces

In reply to Amit Shah

Remove unnecessary braces around a case statement.

Signed-off-by Amit Shah <[email protected]>
---
 hw/virtio-console.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
hw/virtio-console.c +1 -2
index d7fe68b..d0b9354 100644
@@ -48,10 +48,9 @@ static void chr_event(void *opaque, int event)
     VirtConsole *vcon = opaque;
 
     switch (event) {
-    case CHR_EVENT_OPENED: {
+    case CHR_EVENT_OPENED:
         virtio_serial_open(&vcon->port);
         break;
-    }
     case CHR_EVENT_CLOSED:
         virtio_serial_close(&vcon->port);
         break;
Signature · 1 line
1.7.3.2
author
#3 lore raw

PATCH v2 3/4 virtio-serial: Simplify condition for a while loop

In reply to Amit Shah

Separate out a non-changing condition over the period of a loop into an if statement before the loop. This will be used later to re-work the loop.

Signed-off-by Amit Shah <[email protected]>
---
 hw/virtio-serial-bus.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
hw/virtio-serial-bus.c +4 -1
index 74ba5ec..ecf0056 100644
@@ -121,7 +121,10 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
     assert(port || discard);
     assert(virtio_queue_ready(vq));
 
-    while ((discard || !port->throttled) && virtqueue_pop(vq, &elem)) {
+    if (!discard && port->throttled) {
+        return;
+    }
+    while (virtqueue_pop(vq, &elem)) {
         uint8_t *buf;
         size_t ret, buf_size;
Signature · 1 line
1.7.3.2
author
#4 lore raw

PATCH v2 4/4 virtio-serial: Don't copy over guest buffer to host

In reply to Amit Shah

When the guest writes something to a host, we copied over the entire buffer first into the host and then processed it. Do away with that, it could result in a malicious guest causing a DoS on the host.

Reported-by Paul Brook <[email protected]>
Signed-off-by Amit Shah <[email protected]>
---
 hw/virtio-serial-bus.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
hw/virtio-serial-bus.c +8 -8
index ecf0056..a0886a2 100644
@@ -125,16 +125,16 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
         return;
     }
     while (virtqueue_pop(vq, &elem)) {
-        uint8_t *buf;
-        size_t ret, buf_size;
+        unsigned int i;
 
-        if (!discard) {
-            buf_size = iov_size(elem.out_sg, elem.out_num);
-            buf = qemu_malloc(buf_size);
-            ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size);
+        for (i = 0; !discard && i < elem.out_num; i++) {
+            size_t buf_size;
 
-            port->info->have_data(port, buf, ret);
-            qemu_free(buf);
+            buf_size = elem.out_sg[i].iov_len;
+
+            port->info->have_data(port,
+                                  elem.out_sg[i].iov_base,
+                                  buf_size);
         }
         virtqueue_push(vq, &elem, 0);
     }
Signature · 1 line
1.7.3.2