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

login
register
mail settings
Submitter Amit Shah
Date Dec. 10, 2010, 1:25 p.m.
Message ID <38a2b7e584b2d76a6b8d116e9e291edb3b0ef50d.1291987020.git.amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/75091/
State New
Headers show

Comments

Amit Shah - Dec. 10, 2010, 1:25 p.m.
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(-)
Paul Brook - Dec. 10, 2010, 1:59 p.m.
> 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
Amit Shah - Dec. 10, 2010, 2:59 p.m.
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
Paul Brook - Dec. 10, 2010, 3:17 p.m.
> 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
Amit Shah - Dec. 10, 2010, 3:30 p.m.
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

Patch

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