diff mbox

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

Message ID 1268999926-29560-5-git-send-email-amit.shah@redhat.com
State New
Headers show

Commit Message

Amit Shah March 19, 2010, 11:58 a.m. UTC 
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(-)

Comments

Avi Kivity March 20, 2010, 7:40 a.m. UTC | #1 
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. UTC | #2 
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. UTC | #3 
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. UTC | #4 
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. UTC | #5 
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
diff mbox

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);
 }