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

[Qemu-devel] [PATCH 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.

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      |   10 +++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 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 |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 09f62c6..d5b0502 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -673,10 +673,14 @@ 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]);
+    if ((num == 0 && vdev->vq[n].vring.num != 0) ||
+        (num != 0 && vdev->vq[n].vring.num == 0) ||
+        (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 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..aefb7e4 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) == 0) {
+            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 1/2] hw/virtio/virtio: Don't allow guests to add/remove queues

[Michael S. Tsirkin]

On Thu, Jul 25, 2013 at 02:37:42PM +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>
> ---
>  hw/virtio/virtio.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 09f62c6..d5b0502 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -673,10 +673,14 @@ 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]);
> +    if ((num == 0 && vdev->vq[n].vring.num != 0) ||
> +        (num != 0 && vdev->vq[n].vring.num == 0) ||

Cleaner (imho)

    !num != !vdev->vq[n].vring.num


> +        (num > VIRTQUEUE_MAX_SIZE) ||

Pls don't put () around simple math.
It has natural precedence wrt <> so it just makes it
look like lisp.

> +        (num < 0)) {

How does it ever get negative?
assert (num >= 0) instead?

> +        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 2/2] hw/virtio/virtio-mmio: Make QueueNumMax read 0 for unavailable queues

[Michael S. Tsirkin]

On Thu, Jul 25, 2013 at 02:37:43PM +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>
> ---
>  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..aefb7e4 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) == 0) {
> +            return 0;
> +        }

All other callers do:
       if (!virtio_queue_get_num(vdev, vdev->queue_sel)) {

So please make this one consistent and use the shorter form.


>          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 1/2] hw/virtio/virtio: Don't allow guests to add/remove queues

[Peter Maydell]

On 25 July 2013 23:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jul 25, 2013 at 02:37:42PM +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>
>> ---
>>  hw/virtio/virtio.c |   10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 09f62c6..d5b0502 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -673,10 +673,14 @@ 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]);
>> +    if ((num == 0 && vdev->vq[n].vring.num != 0) ||
>> +        (num != 0 && vdev->vq[n].vring.num == 0) ||
>
> Cleaner (imho)
>
>     !num != !vdev->vq[n].vring.num

I think that's more confusing, and you really don't want
"guards so we don't let the guest do bad things" to be
confusing to read.

>> +        (num < 0)) {
>
> How does it ever get negative?

If the guest maliciously writes a value with bit 31 set
to the register...

-- PMM

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

[Peter Maydell]

On 25 July 2013 23:34, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jul 25, 2013 at 02:37:43PM +0100, Peter Maydell wrote:
>>      case VIRTIO_MMIO_QUEUENUMMAX:
>> +        if (virtio_queue_get_num(vdev, vdev->queue_sel) == 0) {
>> +            return 0;
>> +        }
>
> All other callers do:
>        if (!virtio_queue_get_num(vdev, vdev->queue_sel)) {
>
> So please make this one consistent and use the shorter form.

OK.

-- PMM

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

[Michael S. Tsirkin]

On Thu, Jul 25, 2013 at 11:37:22PM +0100, Peter Maydell wrote:
> On 25 July 2013 23:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Jul 25, 2013 at 02:37:42PM +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>
> >> ---
> >>  hw/virtio/virtio.c |   10 +++++++---
> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 09f62c6..d5b0502 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -673,10 +673,14 @@ 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]);
> >> +    if ((num == 0 && vdev->vq[n].vring.num != 0) ||
> >> +        (num != 0 && vdev->vq[n].vring.num == 0) ||
> >
> > Cleaner (imho)
> >
> >     !num != !vdev->vq[n].vring.num
> 
> I think that's more confusing, and you really don't want
> "guards so we don't let the guest do bad things" to be
> confusing to read.

Confusing to whom? That's really subjective.
You can use cast to bool or !! if you prefer.
     (bool)num != (bool)vdev->vq[n].vring.num

Point is, most other code in this file uses (x) and !(x)
and not != 0.
That's objective, so please, find a way to not test ==0/!= 0.

> >> +        (num < 0)) {
> >
> > How does it ever get negative?
> 
> If the guest maliciously writes a value with bit 31 set
> to the register...
> 
> -- PMM

Make the argument unsigned then?

-- 
MST

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

[Peter Maydell]

On 26 July 2013 00:27, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jul 25, 2013 at 11:37:22PM +0100, Peter Maydell wrote:
>> On 25 July 2013 23:33, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Thu, Jul 25, 2013 at 02:37:42PM +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>
>> >> ---
>> >>  hw/virtio/virtio.c |   10 +++++++---
>> >>  1 file changed, 7 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> >> index 09f62c6..d5b0502 100644
>> >> --- a/hw/virtio/virtio.c
>> >> +++ b/hw/virtio/virtio.c
>> >> @@ -673,10 +673,14 @@ 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]);
>> >> +    if ((num == 0 && vdev->vq[n].vring.num != 0) ||
>> >> +        (num != 0 && vdev->vq[n].vring.num == 0) ||
>> >
>> > Cleaner (imho)
>> >
>> >     !num != !vdev->vq[n].vring.num
>>
>> I think that's more confusing, and you really don't want
>> "guards so we don't let the guest do bad things" to be
>> confusing to read.
>
> Confusing to whom? That's really subjective.
> You can use cast to bool or !! if you prefer.
>      (bool)num != (bool)vdev->vq[n].vring.num

This is still confusing. We're trying to say "if the
number is currently zero, don't let it go non-zero;
if it's non-zero, don't let it go zero", and the clear
way to say that is exactly how I wrote it. This isn't
a critical code path so there's no speed justification
for obfuscating what we're doing.

> Point is, most other code in this file uses (x) and !(x)
> and not != 0.
> That's objective, so please, find a way to not test ==0/!= 0.

   if ((!num && vdev->vq[n].vring.num) ||
       (num && !vdev->vq[n].vring.num) ||

>> >> +        (num < 0)) {
>> >
>> > How does it ever get negative?
>>
>> If the guest maliciously writes a value with bit 31 set
>> to the register...

> Make the argument unsigned then?

Would make this function inconsistent with the
existing get_num() function.

-- PMM

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

[Michael S. Tsirkin]

On Fri, Jul 26, 2013 at 09:05:33AM +0100, Peter Maydell wrote:
> On 26 July 2013 00:27, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Jul 25, 2013 at 11:37:22PM +0100, Peter Maydell wrote:
> >> On 25 July 2013 23:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Thu, Jul 25, 2013 at 02:37:42PM +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>
> >> >> ---
> >> >>  hw/virtio/virtio.c |   10 +++++++---
> >> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> >> index 09f62c6..d5b0502 100644
> >> >> --- a/hw/virtio/virtio.c
> >> >> +++ b/hw/virtio/virtio.c
> >> >> @@ -673,10 +673,14 @@ 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]);
> >> >> +    if ((num == 0 && vdev->vq[n].vring.num != 0) ||
> >> >> +        (num != 0 && vdev->vq[n].vring.num == 0) ||
> >> >
> >> > Cleaner (imho)
> >> >
> >> >     !num != !vdev->vq[n].vring.num
> >>
> >> I think that's more confusing, and you really don't want
> >> "guards so we don't let the guest do bad things" to be
> >> confusing to read.
> >
> > Confusing to whom? That's really subjective.
> > You can use cast to bool or !! if you prefer.
> >      (bool)num != (bool)vdev->vq[n].vring.num
> 
> This is still confusing. We're trying to say "if the
> number is currently zero, don't let it go non-zero;
> if it's non-zero, don't let it go zero", and the clear
> way to say that is exactly how I wrote it. This isn't
> a critical code path so there's no speed justification
> for obfuscating what we're doing.

What you write is too low level, you have to squint to
figure out it is correct.  What you are really trying to say is
"don't allow guest change between zero and non zero values".
That's why it's clearer my way: we test "zero" status
with !x (or non zero status with (bool)cast) and make
sure it is not changed.

> > Point is, most other code in this file uses (x) and !(x)
> > and not != 0.
> > That's objective, so please, find a way to not test ==0/!= 0.
> 
>    if ((!num && vdev->vq[n].vring.num) ||
>        (num && !vdev->vq[n].vring.num) ||

Better, though != is still slightly clearer IMO.

> >> >> +        (num < 0)) {
> >> >
> >> > How does it ever get negative?
> >>
> >> If the guest maliciously writes a value with bit 31 set
> >> to the register...
> 
> > Make the argument unsigned then?
> 
> Would make this function inconsistent with the
> existing get_num() function.
> 
> -- PMM

Let's fix that one too?