| Message ID | 1426678962-27545-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 18 Mar 2015 12:42:46 +0100 "Michael S. Tsirkin" <mst@redhat.com> wrote: > All that happens when virtqueue_fill is invoked incorrectly is that we > corrupt guest memory, so this check is not a security measure. > Move the check to ifdef DEBUG to make sure we don't introduce new > crashes close to release. I don't really disagree with this patch, but is anybody actually making regular runs with VIRTIO_DEBUG activated? > Array scans aren't free either, so it's a good idea from performance > point of view, too. > > Cc: Rusty Russell <rusty@rustcorp.com.au> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/virtio/virtio.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 27429c2..37fb2ee 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -243,15 +243,22 @@ int virtio_queue_empty(VirtQueue *vq) > void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len_written, unsigned int idx) > { > - unsigned int offset, tot_wlen; > + unsigned int offset; > int i; > > trace_virtqueue_fill(vq, elem, len_written, idx); > > - for (tot_wlen = i = 0; i < elem->in_num; i++) { > - tot_wlen += elem->in_sg[i].iov_len; > +#ifdef DEBUG_VIRTIO > + { > + /* Check that len_written is <= the writable length. */ > + unsigned int tot_wlen; > + > + for (tot_wlen = i = 0; i < elem->in_num; i++) { > + tot_wlen += elem->in_sg[i].iov_len; > + } > + assert(len_written <= tot_wlen); > } > - assert(len_written <= tot_wlen); > +#endif > > offset = 0; > for (i = 0; i < elem->in_num; i++) {
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 27429c2..37fb2ee 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -243,15 +243,22 @@ int virtio_queue_empty(VirtQueue *vq) void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len_written, unsigned int idx) { - unsigned int offset, tot_wlen; + unsigned int offset; int i; trace_virtqueue_fill(vq, elem, len_written, idx); - for (tot_wlen = i = 0; i < elem->in_num; i++) { - tot_wlen += elem->in_sg[i].iov_len; +#ifdef DEBUG_VIRTIO + { + /* Check that len_written is <= the writable length. */ + unsigned int tot_wlen; + + for (tot_wlen = i = 0; i < elem->in_num; i++) { + tot_wlen += elem->in_sg[i].iov_len; + } + assert(len_written <= tot_wlen); } - assert(len_written <= tot_wlen); +#endif offset = 0; for (i = 0; i < elem->in_num; i++) {
All that happens when virtqueue_fill is invoked incorrectly is that we corrupt guest memory, so this check is not a security measure. Move the check to ifdef DEBUG to make sure we don't introduce new crashes close to release. Array scans aren't free either, so it's a good idea from performance point of view, too. Cc: Rusty Russell <rusty@rustcorp.com.au> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/virtio/virtio.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)