[Qemu-devel] [PATCH 0/5] virtio-serial: Trivial fixes, don't copy buffers to host

[Qemu-devel] [PATCH 0/5] virtio-serial: Trivial fixes, don't copy buffers to host

[Amit Shah]

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

-- 
1.7.3.2

[Qemu-devel] [PATCH 1/5] virtio-console: Factor out common init between console and generic ports

[Amit Shah]

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 <amit.shah@redhat.com>
---
 hw/virtio-console.c |   31 ++++++++++++++-----------------
 1 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index caea11f..d7fe68b 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -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 = {
-- 
1.7.3.2

[Qemu-devel] [PATCH 2/5] virtio-console: Remove unnecessary braces

[Amit Shah]

Remove unnecessary braces around a case statement.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index d7fe68b..d0b9354 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -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;
-- 
1.7.3.2

[Qemu-devel] [PATCH 3/5] virtio-serial: Simplify condition for a while loop

[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 <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 74ba5ec..ecf0056 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -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;
 
-- 
1.7.3.2

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

[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 <paul@codesourcery.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index ecf0056..3bbd915 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -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);
-- 
1.7.3.2

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

[Paul Brook]

> -        if (!discard) {
> +        if (discard) {
> +            goto next;
> +        }

> +    next:
>          virtqueue_push(vq, &elem, 0);

Please don't do this.

Paul

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

[Amit Shah]

On (Fri) Dec 10 2010 [14:02:37], Paul Brook wrote:
> > -        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

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

[Paul Brook]

> On (Fri) Dec 10 2010 [14:02:37], Paul Brook wrote:
> > > -        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.

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

Paul

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

[Amit Shah]

On (Fri) Dec 10 2010 [15:17:18], Paul Brook wrote:
> > On (Fri) Dec 10 2010 [14:02:37], Paul Brook wrote:
> > > > -        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.
> 
> 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

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

[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 <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 3bbd915..3a3032f 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -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
-- 
1.7.3.2

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

[Paul Brook]

> 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

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

[Amit Shah]

On (Fri) Dec 10 2010 [13:59:50], Paul Brook wrote:
> > 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.

> 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

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

[Paul Brook]

> On (Fri) Dec 10 2010 [13:59:50], Paul Brook wrote:
> > > 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.

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

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

[Amit Shah]

On (Fri) Dec 10 2010 [15:17:58], Paul Brook wrote:
> > On (Fri) Dec 10 2010 [13:59:50], Paul Brook wrote:
> > > > 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?

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

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

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