| Message ID | 1374759463-6351-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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?
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
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?
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)
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(-)