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 <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.

> 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