Message ID | 1304890147-26679-1-git-send-email-stefanha@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Sun, May 8, 2011 at 10:29 PM, Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: > The virtio_queue_notify() function checks that the virtqueue number is > less than the maximum number of virtqueues. A signed comparison is used > but the virtqueue number could be negative if a buggy or malicious guest > is run. This results in memory accesses outside of the virtqueue array. > > It is risky doing input validation in common code instead of at the > guest<->host boundary. Note that virtio_queue_set_addr(), > virtio_queue_get_addr(), virtio_queue_get_num(), and many other virtio > functions do *not* validate the virtqueue number argument. > > Instead of fixing the comparison in virtio_queue_notify(), move the > comparison to the virtio bindings (just like VIRTIO_PCI_QUEUE_SEL) where > we have a uint32_t value and can avoid ever calling into common virtio > code if the virtqueue number is invalid. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > --- > hw/syborg_virtio.c | 4 +++- > hw/virtio-pci.c | 4 +++- > hw/virtio.c | 4 +--- > 3 files changed, 7 insertions(+), 5 deletions(-) Please consider this patch for the stable tree once Michael is happy: http://patchwork.ozlabs.org/patch/94604/ Stefan
On Sun, May 08, 2011 at 10:29:07PM +0100, Stefan Hajnoczi wrote: > The virtio_queue_notify() function checks that the virtqueue number is > less than the maximum number of virtqueues. A signed comparison is used > but the virtqueue number could be negative if a buggy or malicious guest > is run. This results in memory accesses outside of the virtqueue array. > > It is risky doing input validation in common code instead of at the > guest<->host boundary. Note that virtio_queue_set_addr(), > virtio_queue_get_addr(), virtio_queue_get_num(), and many other virtio > functions do *not* validate the virtqueue number argument. > > Instead of fixing the comparison in virtio_queue_notify(), move the > comparison to the virtio bindings (just like VIRTIO_PCI_QUEUE_SEL) where > we have a uint32_t value and can avoid ever calling into common virtio > code if the virtqueue number is invalid. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> Looks good to me. Didn't test but Acked-by: Michael S. Tsirkin <mst@redhat.com> Anthony, you are going to merge that? stable trees need that too, probably very far back ... > --- > hw/syborg_virtio.c | 4 +++- > hw/virtio-pci.c | 4 +++- > hw/virtio.c | 4 +--- > 3 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c > index 2f3e6da..00c7be8 100644 > --- a/hw/syborg_virtio.c > +++ b/hw/syborg_virtio.c > @@ -146,7 +146,9 @@ static void syborg_virtio_writel(void *opaque, target_phys_addr_t offset, > vdev->queue_sel = value; > break; > case SYBORG_VIRTIO_QUEUE_NOTIFY: > - virtio_queue_notify(vdev, value); > + if (value < VIRTIO_PCI_QUEUE_MAX) { > + virtio_queue_notify(vdev, value); > + } > break; > case SYBORG_VIRTIO_STATUS: > virtio_set_status(vdev, value & 0xFF); > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index c19629d..6862aa7 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -348,7 +348,9 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > vdev->queue_sel = val; > break; > case VIRTIO_PCI_QUEUE_NOTIFY: > - virtio_queue_notify(vdev, val); > + if (val < VIRTIO_PCI_QUEUE_MAX) { > + virtio_queue_notify(vdev, val); > + } > break; > case VIRTIO_PCI_STATUS: > if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { > diff --git a/hw/virtio.c b/hw/virtio.c > index 6e8814c..a651860 100644 > --- a/hw/virtio.c > +++ b/hw/virtio.c > @@ -585,9 +585,7 @@ void virtio_queue_notify_vq(VirtQueue *vq) > > void virtio_queue_notify(VirtIODevice *vdev, int n) > { > - if (n < VIRTIO_PCI_QUEUE_MAX) { > - virtio_queue_notify_vq(&vdev->vq[n]); > - } > + virtio_queue_notify_vq(&vdev->vq[n]); > } > > uint16_t virtio_queue_vector(VirtIODevice *vdev, int n) > -- > 1.7.4.4
diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c index 2f3e6da..00c7be8 100644 --- a/hw/syborg_virtio.c +++ b/hw/syborg_virtio.c @@ -146,7 +146,9 @@ static void syborg_virtio_writel(void *opaque, target_phys_addr_t offset, vdev->queue_sel = value; break; case SYBORG_VIRTIO_QUEUE_NOTIFY: - virtio_queue_notify(vdev, value); + if (value < VIRTIO_PCI_QUEUE_MAX) { + virtio_queue_notify(vdev, value); + } break; case SYBORG_VIRTIO_STATUS: virtio_set_status(vdev, value & 0xFF); diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index c19629d..6862aa7 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -348,7 +348,9 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) vdev->queue_sel = val; break; case VIRTIO_PCI_QUEUE_NOTIFY: - virtio_queue_notify(vdev, val); + if (val < VIRTIO_PCI_QUEUE_MAX) { + virtio_queue_notify(vdev, val); + } break; case VIRTIO_PCI_STATUS: if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { diff --git a/hw/virtio.c b/hw/virtio.c index 6e8814c..a651860 100644 --- a/hw/virtio.c +++ b/hw/virtio.c @@ -585,9 +585,7 @@ void virtio_queue_notify_vq(VirtQueue *vq) void virtio_queue_notify(VirtIODevice *vdev, int n) { - if (n < VIRTIO_PCI_QUEUE_MAX) { - virtio_queue_notify_vq(&vdev->vq[n]); - } + virtio_queue_notify_vq(&vdev->vq[n]); } uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
The virtio_queue_notify() function checks that the virtqueue number is less than the maximum number of virtqueues. A signed comparison is used but the virtqueue number could be negative if a buggy or malicious guest is run. This results in memory accesses outside of the virtqueue array. It is risky doing input validation in common code instead of at the guest<->host boundary. Note that virtio_queue_set_addr(), virtio_queue_get_addr(), virtio_queue_get_num(), and many other virtio functions do *not* validate the virtqueue number argument. Instead of fixing the comparison in virtio_queue_notify(), move the comparison to the virtio bindings (just like VIRTIO_PCI_QUEUE_SEL) where we have a uint32_t value and can avoid ever calling into common virtio code if the virtqueue number is invalid. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- hw/syborg_virtio.c | 4 +++- hw/virtio-pci.c | 4 +++- hw/virtio.c | 4 +--- 3 files changed, 7 insertions(+), 5 deletions(-)