Patchwork [4/9] virtio-serial: Handle scatter-gather buffers for control messages

login
register
mail settings
Submitter Amit Shah
Date March 19, 2010, 11:58 a.m.
Message ID <1268999926-29560-5-git-send-email-amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/48143/
State New
Headers show

Comments

Amit Shah - March 19, 2010, 11:58 a.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 |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 40 insertions(+), 3 deletions(-)
Avi Kivity - March 20, 2010, 7:40 a.m.
On 03/19/2010 01:58 PM, Amit Shah 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 |   43 ++++++++++++++++++++++++++++++++++++++++---
>   1 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 830eb75..d5887ab 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -200,7 +200,7 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
>   }
>
>   /* 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;
> @@ -208,6 +208,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf)
>       size_t buffer_len;
>
>       gcpkt = buf;
> +    if (len<  sizeof(cpkt)) {
> +        /* The guest sent an invalid control packet */
> +        return;
> +    }
>       port = find_port_by_id(vser, ldl_p(&gcpkt->id));
>       if (!port)
>           return;
> @@ -281,12 +285,45 @@ 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);
> +        unsigned int i;
> +        size_t cur_len, offset;
> +
> +        cur_len = 0;
> +        for (i = 0; i<  elem.out_num; i++) {
> +            cur_len += elem.out_sg[i].iov_len;
> +        }
> +        /*
> +         * 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);
> +        }
> +
> +        offset = 0;
> +        for (i = 0; i<  elem.out_num; i++) {
> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
> +                   elem.out_sg[i].iov_len);
> +            offset += elem.out_sg[i].iov_len;
> +        }
> +        len = cur_len;
> +
> +        handle_control_message(vser, buf, len);
> +        virtqueue_push(vq,&elem, len);
> +    }
> +    if (len) {
> +        qemu_free(buf);
>       }
>       virtio_notify(vdev, vq);
>   }
>    

Isn't there some virtio function to linearize requests?
Amit Shah - March 22, 2010, 5:18 a.m.
On (Sat) Mar 20 2010 [09:40:50], Avi Kivity wrote:
> On 03/19/2010 01:58 PM, Amit Shah wrote:
>> +
>> +        offset = 0;
>> +        for (i = 0; i<  elem.out_num; i++) {
>> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
>> +                   elem.out_sg[i].iov_len);
>> +            offset += elem.out_sg[i].iov_len;
>> +        }
>> +        len = cur_len;
>> +
>> +        handle_control_message(vser, buf, len);
>> +        virtqueue_push(vq,&elem, len);
>> +    }
>> +    if (len) {
>> +        qemu_free(buf);
>>       }
>>       virtio_notify(vdev, vq);
>>   }
>
> Isn't there some virtio function to linearize requests?

I don't see one.

		Amit
Michael S. Tsirkin - March 23, 2010, 3:51 p.m.
On Mon, Mar 22, 2010 at 10:48:02AM +0530, Amit Shah wrote:
> On (Sat) Mar 20 2010 [09:40:50], Avi Kivity wrote:
> > On 03/19/2010 01:58 PM, Amit Shah wrote:
> >> +
> >> +        offset = 0;
> >> +        for (i = 0; i<  elem.out_num; i++) {
> >> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
> >> +                   elem.out_sg[i].iov_len);
> >> +            offset += elem.out_sg[i].iov_len;
> >> +        }
> >> +        len = cur_len;
> >> +
> >> +        handle_control_message(vser, buf, len);
> >> +        virtqueue_push(vq,&elem, len);
> >> +    }
> >> +    if (len) {
> >> +        qemu_free(buf);
> >>       }
> >>       virtio_notify(vdev, vq);
> >>   }
> >
> > Isn't there some virtio function to linearize requests?
> 
> I don't see one.
> 
> 		Amit

virtio-net has iov_fill. Reuse it?
Amit Shah - March 23, 2010, 4:15 p.m.
On (Tue) Mar 23 2010 [17:51:26], Michael S. Tsirkin wrote:
> On Mon, Mar 22, 2010 at 10:48:02AM +0530, Amit Shah wrote:
> > On (Sat) Mar 20 2010 [09:40:50], Avi Kivity wrote:
> > > On 03/19/2010 01:58 PM, Amit Shah wrote:
> > >> +
> > >> +        offset = 0;
> > >> +        for (i = 0; i<  elem.out_num; i++) {
> > >> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
> > >> +                   elem.out_sg[i].iov_len);
> > >> +            offset += elem.out_sg[i].iov_len;
> > >> +        }
> > >> +        len = cur_len;
> > >> +
> > >> +        handle_control_message(vser, buf, len);
> > >> +        virtqueue_push(vq,&elem, len);
> > >> +    }
> > >> +    if (len) {
> > >> +        qemu_free(buf);
> > >>       }
> > >>       virtio_notify(vdev, vq);
> > >>   }
> > >
> > > Isn't there some virtio function to linearize requests?
> > 
> > I don't see one.
> 
> virtio-net has iov_fill. Reuse it?

Hm, yeah. Any ideas on how to share it? Put it in some common file?

Just copying it seems good for now..

		Amit
Michael S. Tsirkin - March 23, 2010, 4:23 p.m.
On Tue, Mar 23, 2010 at 09:45:08PM +0530, Amit Shah wrote:
> On (Tue) Mar 23 2010 [17:51:26], Michael S. Tsirkin wrote:
> > On Mon, Mar 22, 2010 at 10:48:02AM +0530, Amit Shah wrote:
> > > On (Sat) Mar 20 2010 [09:40:50], Avi Kivity wrote:
> > > > On 03/19/2010 01:58 PM, Amit Shah wrote:
> > > >> +
> > > >> +        offset = 0;
> > > >> +        for (i = 0; i<  elem.out_num; i++) {
> > > >> +            memcpy(buf + offset, elem.out_sg[i].iov_base,
> > > >> +                   elem.out_sg[i].iov_len);
> > > >> +            offset += elem.out_sg[i].iov_len;
> > > >> +        }
> > > >> +        len = cur_len;
> > > >> +
> > > >> +        handle_control_message(vser, buf, len);
> > > >> +        virtqueue_push(vq,&elem, len);
> > > >> +    }
> > > >> +    if (len) {
> > > >> +        qemu_free(buf);
> > > >>       }
> > > >>       virtio_notify(vdev, vq);
> > > >>   }
> > > >
> > > > Isn't there some virtio function to linearize requests?
> > > 
> > > I don't see one.
> > 
> > virtio-net has iov_fill. Reuse it?
> 
> Hm, yeah. Any ideas on how to share it? Put it in some common file?
> 
> Just copying it seems good for now..
> 
> 		Amit


Add iov.c

Patch

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 830eb75..d5887ab 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -200,7 +200,7 @@  size_t virtio_serial_guest_ready(VirtIOSerialPort *port)
 }
 
 /* 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;
@@ -208,6 +208,10 @@  static void handle_control_message(VirtIOSerial *vser, void *buf)
     size_t buffer_len;
 
     gcpkt = buf;
+    if (len < sizeof(cpkt)) {
+        /* The guest sent an invalid control packet */
+        return;
+    }
     port = find_port_by_id(vser, ldl_p(&gcpkt->id));
     if (!port)
         return;
@@ -281,12 +285,45 @@  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);
+        unsigned int i;
+        size_t cur_len, offset;
+
+        cur_len = 0;
+        for (i = 0; i < elem.out_num; i++) {
+            cur_len += elem.out_sg[i].iov_len;
+        }
+        /*
+         * 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);
+        }
+
+        offset = 0;
+        for (i = 0; i < elem.out_num; i++) {
+            memcpy(buf + offset, elem.out_sg[i].iov_base,
+                   elem.out_sg[i].iov_len);
+            offset += elem.out_sg[i].iov_len;
+        }
+        len = cur_len;
+
+        handle_control_message(vser, buf, len);
+        virtqueue_push(vq, &elem, len);
+    }
+    if (len) {
+        qemu_free(buf);
     }
     virtio_notify(vdev, vq);
 }