Skip to contentSkip to conversation

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

PATCH Superseded Amit Shah 2 participants 14 messages Dec 10, 2010

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Amit Shah

author
#0 lore raw

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.

Amit Shah (5):
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
virtio-serial: Error out if guest sends unexpected vq elements

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

PATCH 1/5 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 2/5 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 3/5 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 4/5 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 |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)
hw/virtio-serial-bus.c +12 -8
index ecf0056..3bbd915 100644
@@ -125,17 +125,21 @@ 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);
+        if (discard) {
+            goto next;
+        }
+        for (i = 0; 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);
         }
+    next:
         virtqueue_push(vq, &elem, 0);
     }
     virtio_notify(vdev, vq);
Signature · 1 line
1.7.3.2
#5 lore raw

[Qemu-devel] Re: [PATCH 4/5] virtio-serial: Don't copy over guest buffer to host

hw/virtio-serial-bus.c static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq, · line 143

Quoted 7 lines
hw/virtio-serial-bus.c +4 -1
-        if (!discard) {
+        if (discard) {
+            goto next;
+        }
 
+    next:
         virtqueue_push(vq, &elem, 0);

Please don't do this.

Paul

author
#6 lore raw

[Qemu-devel] Re: [PATCH 4/5] virtio-serial: Don't copy over guest buffer to host

Quoted 9 lines · Paul Brook

hw/virtio-serial-bus.c static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq, · line 143

Quoted 7 lines
hw/virtio-serial-bus.c +4 -1
-        if (!discard) {
+        if (discard) {
+            goto next;
+        }
 
+    next:
         virtqueue_push(vq, &elem, 0);

Please don't do this.

Could you elaborate?

I can move the 'discard' check into the following 'for' loop, but since the value of discard doesn't change, I moved it outside.

Amit

#7 lore raw

[Qemu-devel] Re: [PATCH 4/5] virtio-serial: Don't copy over guest buffer to host

Quoted 9 lines · Paul Brook
Quoted 8 lines
Quoted 7 lines
hw/virtio-serial-bus.c +4 -1
-        if (!discard) {
+        if (discard) {
+            goto next;
+        }
 
+    next:
         virtqueue_push(vq, &elem, 0);
Quoted 6 lines
Quoted 1 line

Please don't do this.

Could you elaborate?

I can move the 'discard' check into the following 'for' loop, but since the value of discard doesn't change, I moved it outside.

You've replaced a perfectly good if block with a goto.

Paul

author
#8 lore raw

[Qemu-devel] Re: [PATCH 4/5] virtio-serial: Don't copy over guest buffer to host

Quoted 9 lines · Paul Brook
Quoted 9 lines · Paul Brook
Quoted 8 lines
Quoted 7 lines
hw/virtio-serial-bus.c +4 -1
-        if (!discard) {
+        if (discard) {
+            goto next;
+        }
 
+    next:
         virtqueue_push(vq, &elem, 0);
Quoted 8 lines
Quoted 6 lines
Quoted 1 line

Please don't do this.

Could you elaborate?

I can move the 'discard' check into the following 'for' loop, but since the value of discard doesn't change, I moved it outside.

You've replaced a perfectly good if block with a goto.

To keep the indentation levels low.

I've put it in the for loop for v2.

Amit

author
#9 lore raw

PATCH 5/5 virtio-serial: Error out if guest sends unexpected vq elements

In reply to Amit Shah

Check if the guest really sent any items in the out_vq before using them. Similarly, check if there is a buffer to send data in before writing.

Signed-off-by Amit Shah <[email protected]>
---
 hw/virtio-serial-bus.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)
hw/virtio-serial-bus.c +19 -0
index 3bbd915..3a3032f 100644
@@ -102,6 +102,11 @@ static size_t write_to_port(VirtIOSerialPort *port,
             break;
         }
 
+        if (elem.in_num < 1) {
+            error_report("No buffer to send data in?");
+            abort();
+        }
+
         len = iov_from_buf(elem.in_sg, elem.in_num,
                            buf + offset, size - offset);
         offset += len;

@@ -127,6 +132,11 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
     while (virtqueue_pop(vq, &elem)) {
         unsigned int i;
 
+        if (elem.out_num < 1) {
+            error_report("No data sent by guest?");
+            abort();
+        }
+
         if (discard) {
             goto next;
         }

@@ -169,6 +179,11 @@ static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)
         return 0;
     }
 
+    if (elem.in_num < 1) {
+        error_report("No buffer to send control data in?");
+        abort();
+    }
+
     cpkt = (struct virtio_console_control *)buf;
     stl_p(&cpkt->id, port->id);
     memcpy(elem.in_sg[0].iov_base, buf, len);

@@ -386,6 +401,10 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
     while (virtqueue_pop(vq, &elem)) {
         size_t cur_len, copied;
 
+        if (elem.out_num < 1) {
+            error_report("No data sent in control packet");
+            abort();
+        }
         cur_len = iov_size(elem.out_sg, elem.out_num);
         /*
          * Allocate a new buf only if we didn't have one previously or
Signature · 1 line
1.7.3.2
#10 lore raw

[Qemu-devel] Re: [PATCH 5/5] virtio-serial: Error out if guest sends unexpected vq elements

Quoted 3 lines

Check if the guest really sent any items in the out_vq before using them. Similarly, check if there is a buffer to send data in before writing.

Can this actually happen? If so why/how?
Why does it need a special case in this device?

If this is guest triggerable then calling abort() is wrong.

Paul

author
#11 lore raw

[Qemu-devel] Re: [PATCH 5/5] virtio-serial: Error out if guest sends unexpected vq elements

Quoted 6 lines · Paul Brook
Quoted 3 lines

Check if the guest really sent any items in the out_vq before using them. Similarly, check if there is a buffer to send data in before writing.

Can this actually happen? If so why/how?
Why does it need a special case in this device?

A malicious guest (ie, a guest with the virtio_console module suitably modified) could send in buffers with the 'input' bit set instead of output as expected or vice-versa.

Quoted 1 line

If this is guest triggerable then calling abort() is wrong.

It's either a guest bug or a malicious guest. What action is recommended?

Amit

#12 lore raw

[Qemu-devel] Re: [PATCH 5/5] virtio-serial: Error out if guest sends unexpected vq elements

Quoted 11 lines · Paul Brook
Quoted 6 lines
Quoted 3 lines

Check if the guest really sent any items in the out_vq before using them. Similarly, check if there is a buffer to send data in before writing.

Can this actually happen? If so why/how?
Why does it need a special case in this device?

A malicious guest (ie, a guest with the virtio_console module suitably modified) could send in buffers with the 'input' bit set instead of output as expected or vice-versa.

So what? Who cares if they get it wrong?

It's entirely unclear whether this is actually an error. If a request has zero size then we just transfer zero bytes, exactly as requested.

Even if you accept this should be a diagnosable error, I suspect your patch is still insufficient. I don't see any code to check that input queue requests have zero output segments, nor do I see anything to handle zero-length segments.

Quoted 4 lines
Quoted 1 line

If this is guest triggerable then calling abort() is wrong.

It's either a guest bug or a malicious guest. What action is recommended?

Killing the whole VM in response to a malformed request to a device is clearly a bug in that device. You should report an error to the guest in the normal manner. IIRC virtio lacks any consistent error reporting mechanisms, and the usual response when asked to do something impossible is to reset the device.

Paul

author
#13 lore raw

[Qemu-devel] Re: [PATCH 5/5] virtio-serial: Error out if guest sends unexpected vq elements

Quoted 5 lines · Paul Brook
Quoted 5 lines · Paul Brook
Quoted 4 lines
Quoted 3 lines

Check if the guest really sent any items in the out_vq before using them. Similarly, check if there is a buffer to send data in before writing.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

Can this actually happen? If so why/how?
Why does it need a special case in this device?

A malicious guest (ie, a guest with the virtio_console module suitably modified) could send in buffers with the 'input' bit set instead of output as expected or vice-versa.

So what? Who cares if they get it wrong?

Just let the error_report() be there and continue as if nothing happened?

Quoted 7 lines

It's entirely unclear whether this is actually an error. If a request has zero size then we just transfer zero bytes, exactly as requested.

Even if you accept this should be a diagnosable error, I suspect your patch is still insufficient. I don't see any code to check that input queue requests have zero output segments, nor do I see anything to handle zero-length segments.

virtio actually supports sending both, input as well as output types of buffers in one go.

Quoted 9 lines
Quoted 4 lines
Quoted 1 line

If this is guest triggerable then calling abort() is wrong.

It's either a guest bug or a malicious guest. What action is recommended?

Killing the whole VM in response to a malformed request to a device is clearly a bug in that device. You should report an error to the guest in the normal manner. IIRC virtio lacks any consistent error reporting mechanisms, and the usual response when asked to do something impossible is to reset the device.

OK, agreed.

Amit