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 <address@hidden> wrote:
> > On Thu, Jul 25, 2013 at 11:37:22PM +0100, Peter Maydell wrote:
> >> On 25 July 2013 23:33, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
> >> >> ---
> >> >>  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?