[Qemu-devel] Re: [PATCH 14/15] virtio-serial: Handle scatter-gather buffers for control messages

[Juan Quintela <quintela@redhat.com>]

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.