[Qemu-devel] [PATCH v2 for-1.6 0/2] virtio-mmio: fixes to QueueNum, QueueNumMax

[Qemu-devel] [PATCH v2 for-1.6 0/2] virtio-mmio: fixes to QueueNum, QueueNumMax

[Peter Maydell]

These patches fix a couple of bugs in virtio-mmio's
handling of the registers that deal with the queue size:

 * as mst points out, letting the guest flip a queue between
   "exists" and "doesn't exist" is a bad idea
 * QueueNumMax wasn't reading the correct value for nonexistent
   queues

This doesn't include any change to the behaviour of queuesize
on reset (discussed in other thread); the current behaviour is
not a problem for well-behaved guests, and safe in the face
of badly-behaved guests, and currently improving the reset
behaviour is blocked by an unrelated bug.

v1->v2: changes as per mst review:
 * avoid explicit "== 0" comparisons
 * avoid unnecessary parens round comparison ops
 * do the "don't flip between existent and nonexistent" check
   with "!!num != !!oldnum" (and add a comment noting why we're
   doing this check)

Peter Maydell (2):
  hw/virtio/virtio: Don't allow guests to add/remove queues
  hw/virtio/virtio-mmio: Make QueueNumMax read 0 for unavailable queues

 hw/virtio/virtio-mmio.c |    3 +++
 hw/virtio/virtio.c      |   12 +++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH v2 for-1.6 1/2] hw/virtio/virtio: Don't allow guests to add/remove queues

[Peter Maydell]

A queue size of 0 is used to indicate a nonexistent queue, so
don't allow the guest to flip a queue between zero-size and
non-zero-size. Don't permit setting of negative queue sizes
either.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/virtio/virtio.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 09f62c6..60653f7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -673,10 +673,16 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
 
 void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
 {
-    if (num <= VIRTQUEUE_MAX_SIZE) {
-        vdev->vq[n].vring.num = num;
-        virtqueue_init(&vdev->vq[n]);
+    /* Don't allow guest to flip queue between existent and
+     * nonexistent states, or to set it to an invalid size.
+     */
+    if (!!num != !!vdev->vq[n].vring.num ||
+        num > VIRTQUEUE_MAX_SIZE ||
+        num < 0) {
+        return;
     }
+    vdev->vq[n].vring.num = num;
+    virtqueue_init(&vdev->vq[n]);
 }
 
 int virtio_queue_get_num(VirtIODevice *vdev, int n)
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v2 for-1.6 1/2] hw/virtio/virtio: Don't allow guests to add/remove queues

[Michael S. Tsirkin]

On Fri, Jul 26, 2013 at 04:41:27PM +0100, Peter Maydell wrote:
> A queue size of 0 is used to indicate a nonexistent queue, so
> don't allow the guest to flip a queue between zero-size and
> non-zero-size. Don't permit setting of negative queue sizes
> either.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/virtio/virtio.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 09f62c6..60653f7 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -673,10 +673,16 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
>  
>  void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
>  {
> -    if (num <= VIRTQUEUE_MAX_SIZE) {
> -        vdev->vq[n].vring.num = num;
> -        virtqueue_init(&vdev->vq[n]);
> +    /* Don't allow guest to flip queue between existent and
> +     * nonexistent states, or to set it to an invalid size.
> +     */
> +    if (!!num != !!vdev->vq[n].vring.num ||
> +        num > VIRTQUEUE_MAX_SIZE ||
> +        num < 0) {
> +        return;
>      }
> +    vdev->vq[n].vring.num = num;
> +    virtqueue_init(&vdev->vq[n]);
>  }
>  
>  int virtio_queue_get_num(VirtIODevice *vdev, int n)
> -- 
> 1.7.9.5

[Qemu-devel] [PATCH v2 for-1.6 2/2] hw/virtio/virtio-mmio: Make QueueNumMax read 0 for unavailable queues

[Peter Maydell]

The virtio-mmio spec says that QueueNumMax must read zero for queues
which are unavailable; implement this, rather than always returning
VIRTQUEUE_MAX_SIZE.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/virtio/virtio-mmio.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 54d6679..9cf79ce 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -151,6 +151,9 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
         }
         return proxy->host_features;
     case VIRTIO_MMIO_QUEUENUMMAX:
+        if (!virtio_queue_get_num(vdev, vdev->queue_sel)) {
+            return 0;
+        }
         return VIRTQUEUE_MAX_SIZE;
     case VIRTIO_MMIO_QUEUEPFN:
         return virtio_queue_get_addr(vdev, vdev->queue_sel)
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v2 for-1.6 2/2] hw/virtio/virtio-mmio: Make QueueNumMax read 0 for unavailable queues

[Michael S. Tsirkin]

On Fri, Jul 26, 2013 at 04:41:28PM +0100, Peter Maydell wrote:
> The virtio-mmio spec says that QueueNumMax must read zero for queues
> which are unavailable; implement this, rather than always returning
> VIRTQUEUE_MAX_SIZE.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  hw/virtio/virtio-mmio.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 54d6679..9cf79ce 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -151,6 +151,9 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>          }
>          return proxy->host_features;
>      case VIRTIO_MMIO_QUEUENUMMAX:
> +        if (!virtio_queue_get_num(vdev, vdev->queue_sel)) {
> +            return 0;
> +        }
>          return VIRTQUEUE_MAX_SIZE;
>      case VIRTIO_MMIO_QUEUEPFN:
>          return virtio_queue_get_addr(vdev, vdev->queue_sel)
> -- 
> 1.7.9.5

Re: [Qemu-devel] [PATCH v2 for-1.6 0/2] virtio-mmio: fixes to QueueNum, QueueNumMax

[Peter Maydell]

On 26 July 2013 16:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> These patches fix a couple of bugs in virtio-mmio's
> handling of the registers that deal with the queue size:
>
>  * as mst points out, letting the guest flip a queue between
>    "exists" and "doesn't exist" is a bad idea
>  * QueueNumMax wasn't reading the correct value for nonexistent
>    queues
>
> This doesn't include any change to the behaviour of queuesize
> on reset (discussed in other thread); the current behaviour is
> not a problem for well-behaved guests, and safe in the face
> of badly-behaved guests, and currently improving the reset
> behaviour is blocked by an unrelated bug.
>
> v1->v2: changes as per mst review:
>  * avoid explicit "== 0" comparisons
>  * avoid unnecessary parens round comparison ops
>  * do the "don't flip between existent and nonexistent" check
>    with "!!num != !!oldnum" (and add a comment noting why we're
>    doing this check)
>
> Peter Maydell (2):
>   hw/virtio/virtio: Don't allow guests to add/remove queues
>   hw/virtio/virtio-mmio: Make QueueNumMax read 0 for unavailable queues

These didn't make it into 1.6, but in the absence of any
review comments I'm putting them into arm-devs for post-1.6.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 for-1.6 0/2] virtio-mmio: fixes to QueueNum, QueueNumMax

[Michael S. Tsirkin]

On Fri, Aug 09, 2013 at 04:47:40PM +0100, Peter Maydell wrote:
> On 26 July 2013 16:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> > These patches fix a couple of bugs in virtio-mmio's
> > handling of the registers that deal with the queue size:
> >
> >  * as mst points out, letting the guest flip a queue between
> >    "exists" and "doesn't exist" is a bad idea
> >  * QueueNumMax wasn't reading the correct value for nonexistent
> >    queues
> >
> > This doesn't include any change to the behaviour of queuesize
> > on reset (discussed in other thread); the current behaviour is
> > not a problem for well-behaved guests, and safe in the face
> > of badly-behaved guests, and currently improving the reset
> > behaviour is blocked by an unrelated bug.
> >
> > v1->v2: changes as per mst review:
> >  * avoid explicit "== 0" comparisons
> >  * avoid unnecessary parens round comparison ops
> >  * do the "don't flip between existent and nonexistent" check
> >    with "!!num != !!oldnum" (and add a comment noting why we're
> >    doing this check)
> >
> > Peter Maydell (2):
> >   hw/virtio/virtio: Don't allow guests to add/remove queues
> >   hw/virtio/virtio-mmio: Make QueueNumMax read 0 for unavailable queues
> 
> These didn't make it into 1.6, but in the absence of any
> review comments I'm putting them into arm-devs for post-1.6.
> 
> thanks
> -- PMM

I'd say these are important bugfixes, should be OK for 1.6 still.