[Qemu-devel] [PATCH] virtio: move sanity checks to ifdef DEBUG

[Qemu-devel] [PATCH] virtio: move sanity checks to ifdef DEBUG

[Michael S. Tsirkin]

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(-)

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++) {
-- 
MST

Re: [Qemu-devel] [PATCH] virtio: move sanity checks to ifdef DEBUG

[Cornelia Huck]

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++) {