| Message ID | 1269442173-18421-15-git-send-email-amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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); }
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(-)