Patchwork [1/2] hw/virtio/virtio: Don't allow guests to add/remove queues

login
register
mail settings
Submitter Peter Maydell
Date July 25, 2013, 1:37 p.m.
Message ID <1374759463-6351-2-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/261699/
State New
Headers show

Comments

Peter Maydell - July 25, 2013, 1:37 p.m.
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(-)
Michael S. Tsirkin - July 25, 2013, 10:33 p.m.
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
Peter Maydell - July 25, 2013, 10:37 p.m.
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
Michael S. Tsirkin - July 25, 2013, 11:27 p.m.
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?
Peter Maydell - July 26, 2013, 8:05 a.m.
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
Michael S. Tsirkin - July 28, 2013, 6:54 a.m.
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?

Patch

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)