Patchwork [14/15] virtio-serial: Handle scatter-gather buffers for control messages

login
register
mail settings
Submitter Amit Shah
Date March 24, 2010, 2:49 p.m.
Message ID <1269442173-18421-15-git-send-email-amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/48433/
State New
Headers show

Comments

Amit Shah - March 24, 2010, 2:49 p.m.
Current control messages are small enough to not be split into multiple
buffers but we could run into such a situation in the future or a
malicious guest could cause such a situation.

So handle the entire iov request for control messages.

Also ensure the size of the control request is >= what we expect
otherwise we risk accessing memory that we don't own.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
CC: Avi Kivity <avi@redhat.com>
Reported-by: Avi Kivity <avi@redhat.com>
---
 hw/virtio-serial-bus.c |   34 +++++++++++++++++++++++++++++++---
 1 files changed, 31 insertions(+), 3 deletions(-)
Juan Quintela - March 30, 2010, 1:44 p.m.
Amit Shah <amit.shah@redhat.com> wrote:
> Current control messages are small enough to not be split into multiple
> buffers but we could run into such a situation in the future or a
> malicious guest could cause such a situation.
>
> So handle the entire iov request for control messages.
>
> Also ensure the size of the control request is >= what we expect
> otherwise we risk accessing memory that we don't own.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> CC: Avi Kivity <avi@redhat.com>
> Reported-by: Avi Kivity <avi@redhat.com>
> ---
>  hw/virtio-serial-bus.c |   34 +++++++++++++++++++++++++++++++---
>  1 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index bd1223e..3edfeca 100644
>      vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
>  
> +    len = 0;
> +    buf = NULL;
>      while (virtqueue_pop(vq, &elem)) {
> -        handle_control_message(vser, elem.out_sg[0].iov_base);
> -        virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
> +        size_t cur_len, copied;
> +
> +        cur_len = iov_size(elem.out_sg, elem.out_num);
> +        /*
> +         * Allocate a new buf only if we didn't have one previously or
> +         * if the size of the buf differs
> +         */
> +        if (cur_len != len) {
> +            if (len) {
> +                qemu_free(buf);
> +            }
> +            buf = qemu_malloc(cur_len);
> +            len = cur_len;
> +        }

This can be simplified to only allocate the buffer if it is less no?

        if (cur_len > len) {
            if (len) {
                qemu_free(buf);
            }
            buf = qemu_malloc(cur_len);
            len = cur_len;
        }

This way we can elliminate allocations, no?

Later, Juan.
Amit Shah - March 30, 2010, 1:47 p.m.
On (Tue) Mar 30 2010 [15:44:21], Juan Quintela wrote:
> Amit Shah <amit.shah@redhat.com> wrote:
> > Current control messages are small enough to not be split into multiple
> > buffers but we could run into such a situation in the future or a
> > malicious guest could cause such a situation.
> >
> > So handle the entire iov request for control messages.
> >
> > Also ensure the size of the control request is >= what we expect
> > otherwise we risk accessing memory that we don't own.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > CC: Avi Kivity <avi@redhat.com>
> > Reported-by: Avi Kivity <avi@redhat.com>
> > ---
> >  hw/virtio-serial-bus.c |   34 +++++++++++++++++++++++++++++++---
> >  1 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > index bd1223e..3edfeca 100644
> >      vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
> >  
> > +    len = 0;
> > +    buf = NULL;
> >      while (virtqueue_pop(vq, &elem)) {
> > -        handle_control_message(vser, elem.out_sg[0].iov_base);
> > -        virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
> > +        size_t cur_len, copied;
> > +
> > +        cur_len = iov_size(elem.out_sg, elem.out_num);
> > +        /*
> > +         * Allocate a new buf only if we didn't have one previously or
> > +         * if the size of the buf differs
> > +         */
> > +        if (cur_len != len) {
> > +            if (len) {
> > +                qemu_free(buf);
> > +            }
> > +            buf = qemu_malloc(cur_len);
> > +            len = cur_len;
> > +        }
> 
> This can be simplified to only allocate the buffer if it is less no?

Currently all the control messages are the same size, sizeof(struct
virtio_console_control), so it wouldn't matter.

But I guess this could be done, just have to ensure we don't leak data
meant for one control message to another.

		Amit

Patch

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index bd1223e..3edfeca 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -216,7 +216,7 @@  static void mon_event(const char *device, const uint32_t port_id,
 }
 
 /* Guest wants to notify us of some event */
-static void handle_control_message(VirtIOSerial *vser, void *buf)
+static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
 {
     struct VirtIOSerialPort *port;
     struct virtio_console_control cpkt, *gcpkt;
@@ -225,6 +225,11 @@  static void handle_control_message(VirtIOSerial *vser, void *buf)
 
     gcpkt = buf;
 
+    if (len < sizeof(cpkt)) {
+        /* The guest sent an invalid control packet */
+        return;
+    }
+
     cpkt.event = lduw_p(&gcpkt->event);
     cpkt.value = lduw_p(&gcpkt->value);
 
@@ -321,12 +326,35 @@  static void control_out(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtQueueElement elem;
     VirtIOSerial *vser;
+    uint8_t *buf;
+    size_t len;
 
     vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
 
+    len = 0;
+    buf = NULL;
     while (virtqueue_pop(vq, &elem)) {
-        handle_control_message(vser, elem.out_sg[0].iov_base);
-        virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
+        size_t cur_len, copied;
+
+        cur_len = iov_size(elem.out_sg, elem.out_num);
+        /*
+         * Allocate a new buf only if we didn't have one previously or
+         * if the size of the buf differs
+         */
+        if (cur_len != len) {
+            if (len) {
+                qemu_free(buf);
+            }
+            buf = qemu_malloc(cur_len);
+            len = cur_len;
+        }
+        copied = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, len);
+
+        handle_control_message(vser, buf, copied);
+        virtqueue_push(vq, &elem, copied);
+    }
+    if (len) {
+        qemu_free(buf);
     }
     virtio_notify(vdev, vq);
 }