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

[Avi Kivity <avi@redhat.com>]

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?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.