| Message ID | 20220303231050.2146621-4-martin.b.radev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix few small issues in virtio code | expand |
Hi, On Fri, Mar 04, 2022 at 01:10:48AM +0200, Martin Radev wrote: > This patch checks for overflows in QUEUE_NOTIFY and QUEUE_SEL in > the PCI and MMIO operation handling paths. Further, the return > value type of get_vq_count is changed from int to uint since negative > doesn't carry any semantic meaning. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > include/kvm/virtio.h | 2 +- > virtio/9p.c | 2 +- > virtio/balloon.c | 2 +- > virtio/blk.c | 2 +- > virtio/console.c | 2 +- > virtio/mmio.c | 20 ++++++++++++++++++-- > virtio/net.c | 4 ++-- > virtio/pci.c | 21 ++++++++++++++++++--- > virtio/rng.c | 2 +- > virtio/scsi.c | 2 +- > virtio/vsock.c | 2 +- > 11 files changed, 46 insertions(+), 15 deletions(-) > > diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h > index 3880e74..ad274ac 100644 > --- a/include/kvm/virtio.h > +++ b/include/kvm/virtio.h > @@ -187,7 +187,7 @@ struct virtio_ops { > size_t (*get_config_size)(struct kvm *kvm, void *dev); > u32 (*get_host_features)(struct kvm *kvm, void *dev); > void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); > - int (*get_vq_count)(struct kvm *kvm, void *dev); > + unsigned int (*get_vq_count)(struct kvm *kvm, void *dev); > int (*init_vq)(struct kvm *kvm, void *dev, u32 vq, u32 page_size, > u32 align, u32 pfn); > void (*exit_vq)(struct kvm *kvm, void *dev, u32 vq); > diff --git a/virtio/9p.c b/virtio/9p.c > index 6074f3a..7374f1e 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -1469,7 +1469,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static unsigned int get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/balloon.c b/virtio/balloon.c > index 5bcd6ab..450b36a 100644 > --- a/virtio/balloon.c > +++ b/virtio/balloon.c > @@ -251,7 +251,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static unsigned int get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/blk.c b/virtio/blk.c > index af71c0c..46ee028 100644 > --- a/virtio/blk.c > +++ b/virtio/blk.c > @@ -291,7 +291,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static unsigned int get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/console.c b/virtio/console.c > index dae6034..8315808 100644 > --- a/virtio/console.c > +++ b/virtio/console.c > @@ -216,7 +216,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static unsigned int get_vq_count(struct kvm *kvm, void *dev) > { > return VIRTIO_CONSOLE_NUM_QUEUES; > } > diff --git a/virtio/mmio.c b/virtio/mmio.c > index 0094856..d3555b4 100644 > --- a/virtio/mmio.c > +++ b/virtio/mmio.c > @@ -175,13 +175,22 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, > { > struct virtio_mmio *vmmio = vdev->virtio; > struct kvm *kvm = vmmio->kvm; > + unsigned int vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); > u32 val = 0; > > switch (addr) { > case VIRTIO_MMIO_HOST_FEATURES_SEL: > case VIRTIO_MMIO_GUEST_FEATURES_SEL: > + val = ioport__read32(data); > + *(u32 *)(((void *)&vmmio->hdr) + addr) = val; > + break; > case VIRTIO_MMIO_QUEUE_SEL: > val = ioport__read32(data); > + if (val >= vq_count) { > + WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + break; > + } > *(u32 *)(((void *)&vmmio->hdr) + addr) = val; > break; > case VIRTIO_MMIO_STATUS: > @@ -227,6 +236,11 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, > break; > case VIRTIO_MMIO_QUEUE_NOTIFY: > val = ioport__read32(data); > + if (val >= vq_count) { > + WARN_ONCE(1, "QUEUE_NOTIFY value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + break; > + } > vdev->ops->notify_vq(vmmio->kvm, vmmio->dev, val); > break; > case VIRTIO_MMIO_INTERRUPT_ACK: > @@ -346,10 +360,12 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev) > { > - int vq; > + unsigned int vq; > struct virtio_mmio *vmmio = vdev->virtio; > + unsigned int vq_count; > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++) > + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); > + for (vq = 0; vq < vq_count; vq++) Nitpick: this change is unnecessary and pollutes the git history for this file. Same for virtio_pci_reset() below. > virtio_mmio_exit_vq(kvm, vdev, vq); > > return 0; > diff --git a/virtio/net.c b/virtio/net.c > index ec5dc1f..8dd523f 100644 > --- a/virtio/net.c > +++ b/virtio/net.c > @@ -755,11 +755,11 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static unsigned int get_vq_count(struct kvm *kvm, void *dev) > { > struct net_dev *ndev = dev; > > - return ndev->queue_pairs * 2 + 1; > + return ndev->queue_pairs * 2U + 1U; I don't think the cast is needed, as far as I know signed integers are converted to unsigned integers as far back as C89 (and probably even before that). Other than the nitpicks above, the patch looks good. Thanks, Alex > } > > static struct virtio_ops net_dev_virtio_ops = { > diff --git a/virtio/pci.c b/virtio/pci.c > index 0b5cccd..9a6cbf3 100644 > --- a/virtio/pci.c > +++ b/virtio/pci.c > @@ -329,9 +329,11 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde > struct virtio_pci *vpci; > struct kvm *kvm; > u32 val; > + unsigned int vq_count; > > kvm = vcpu->kvm; > vpci = vdev->virtio; > + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); > > switch (offset) { > case VIRTIO_PCI_GUEST_FEATURES: > @@ -351,10 +353,21 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde > } > break; > case VIRTIO_PCI_QUEUE_SEL: > - vpci->queue_selector = ioport__read16(data); > + val = ioport__read16(data); > + if (val >= vq_count) { > + WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + return false; > + } > + vpci->queue_selector = val; > break; > case VIRTIO_PCI_QUEUE_NOTIFY: > val = ioport__read16(data); > + if (val >= vq_count) { > + WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + return false; > + } > vdev->ops->notify_vq(kvm, vpci->dev, val); > break; > case VIRTIO_PCI_STATUS: > @@ -647,10 +660,12 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev) > { > - int vq; > + unsigned int vq; > + unsigned int vq_count; > struct virtio_pci *vpci = vdev->virtio; > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++) > + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); > + for (vq = 0; vq < vq_count; vq++) > virtio_pci_exit_vq(kvm, vdev, vq); > > return 0; > diff --git a/virtio/rng.c b/virtio/rng.c > index c7835a0..75b682e 100644 > --- a/virtio/rng.c > +++ b/virtio/rng.c > @@ -147,7 +147,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static unsigned int get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/scsi.c b/virtio/scsi.c > index 8f1c348..60432cc 100644 > --- a/virtio/scsi.c > +++ b/virtio/scsi.c > @@ -176,7 +176,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static unsigned int get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/vsock.c b/virtio/vsock.c > index 34397b6..64b4e95 100644 > --- a/virtio/vsock.c > +++ b/virtio/vsock.c > @@ -204,7 +204,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi) > die_perror("VHOST_SET_VRING_CALL failed"); > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static unsigned int get_vq_count(struct kvm *kvm, void *dev) > { > return VSOCK_VQ_MAX; > } > -- > 2.25.1 >
Thanks for the review. Comments are inline. Here is the patch: From dc2b54f3368bd012eaa2f55bb8b98278f6278df1 Mon Sep 17 00:00:00 2001 From: Martin Radev <martin.b.radev@gmail.com> Date: Sun, 16 Jan 2022 18:50:44 +0200 Subject: [PATCH kvmtool 5/6] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL This patch checks for overflows in QUEUE_NOTIFY and QUEUE_SEL in the PCI and MMIO operation handling paths. Further, the return value type of get_vq_count is changed from int to uint since negative doesn't carry any semantic meaning. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- include/kvm/virtio.h | 2 +- virtio/9p.c | 2 +- virtio/balloon.c | 2 +- virtio/blk.c | 2 +- virtio/console.c | 2 +- virtio/mmio.c | 16 +++++++++++++++- virtio/net.c | 2 +- virtio/pci.c | 17 +++++++++++++++-- virtio/rng.c | 2 +- virtio/scsi.c | 2 +- virtio/vsock.c | 2 +- 11 files changed, 39 insertions(+), 12 deletions(-) diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h index 3880e74..ad274ac 100644 --- a/include/kvm/virtio.h +++ b/include/kvm/virtio.h @@ -187,7 +187,7 @@ struct virtio_ops { size_t (*get_config_size)(struct kvm *kvm, void *dev); u32 (*get_host_features)(struct kvm *kvm, void *dev); void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); - int (*get_vq_count)(struct kvm *kvm, void *dev); + unsigned int (*get_vq_count)(struct kvm *kvm, void *dev); int (*init_vq)(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align, u32 pfn); void (*exit_vq)(struct kvm *kvm, void *dev, u32 vq); diff --git a/virtio/9p.c b/virtio/9p.c index 57cd6d0..7c9d792 100644 --- a/virtio/9p.c +++ b/virtio/9p.c @@ -1469,7 +1469,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/balloon.c b/virtio/balloon.c index 5bcd6ab..450b36a 100644 --- a/virtio/balloon.c +++ b/virtio/balloon.c @@ -251,7 +251,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/blk.c b/virtio/blk.c index af71c0c..46ee028 100644 --- a/virtio/blk.c +++ b/virtio/blk.c @@ -291,7 +291,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/console.c b/virtio/console.c index dae6034..8315808 100644 --- a/virtio/console.c +++ b/virtio/console.c @@ -216,7 +216,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return VIRTIO_CONSOLE_NUM_QUEUES; } diff --git a/virtio/mmio.c b/virtio/mmio.c index c14f08a..4c16359 100644 --- a/virtio/mmio.c +++ b/virtio/mmio.c @@ -175,13 +175,22 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, { struct virtio_mmio *vmmio = vdev->virtio; struct kvm *kvm = vmmio->kvm; + unsigned int vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); u32 val = 0; switch (addr) { case VIRTIO_MMIO_HOST_FEATURES_SEL: case VIRTIO_MMIO_GUEST_FEATURES_SEL: + val = ioport__read32(data); + *(u32 *)(((void *)&vmmio->hdr) + addr) = val; + break; case VIRTIO_MMIO_QUEUE_SEL: val = ioport__read32(data); + if (val >= vq_count) { + WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n", + val, vq_count); + break; + } *(u32 *)(((void *)&vmmio->hdr) + addr) = val; break; case VIRTIO_MMIO_STATUS: @@ -227,6 +236,11 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, break; case VIRTIO_MMIO_QUEUE_NOTIFY: val = ioport__read32(data); + if (val >= vq_count) { + WARN_ONCE(1, "QUEUE_NOTIFY value (%u) is larger than VQ count (%u)\n", + val, vq_count); + break; + } vdev->ops->notify_vq(vmmio->kvm, vmmio->dev, val); break; case VIRTIO_MMIO_INTERRUPT_ACK: @@ -346,7 +360,7 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev, int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev) { - int vq; + unsigned int vq; struct virtio_mmio *vmmio = vdev->virtio; for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++) diff --git a/virtio/net.c b/virtio/net.c index ec5dc1f..67070d6 100644 --- a/virtio/net.c +++ b/virtio/net.c @@ -755,7 +755,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { struct net_dev *ndev = dev; diff --git a/virtio/pci.c b/virtio/pci.c index 13f2b76..eb7af32 100644 --- a/virtio/pci.c +++ b/virtio/pci.c @@ -320,9 +320,11 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde struct virtio_pci *vpci; struct kvm *kvm; u32 val; + unsigned int vq_count; kvm = vcpu->kvm; vpci = vdev->virtio; + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); switch (offset) { case VIRTIO_PCI_GUEST_FEATURES: @@ -342,10 +344,21 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde } break; case VIRTIO_PCI_QUEUE_SEL: - vpci->queue_selector = ioport__read16(data); + val = ioport__read16(data); + if (val >= vq_count) { + WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n", + val, vq_count); + return false; + } + vpci->queue_selector = val; break; case VIRTIO_PCI_QUEUE_NOTIFY: val = ioport__read16(data); + if (val >= vq_count) { + WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n", + val, vq_count); + return false; + } vdev->ops->notify_vq(kvm, vpci->dev, val); break; case VIRTIO_PCI_STATUS: @@ -638,7 +651,7 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev, int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev) { - int vq; + unsigned int vq; struct virtio_pci *vpci = vdev->virtio; for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++) diff --git a/virtio/rng.c b/virtio/rng.c index c7835a0..75b682e 100644 --- a/virtio/rng.c +++ b/virtio/rng.c @@ -147,7 +147,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/scsi.c b/virtio/scsi.c index 8f1c348..60432cc 100644 --- a/virtio/scsi.c +++ b/virtio/scsi.c @@ -176,7 +176,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/vsock.c b/virtio/vsock.c index 34397b6..64b4e95 100644 --- a/virtio/vsock.c +++ b/virtio/vsock.c @@ -204,7 +204,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi) die_perror("VHOST_SET_VRING_CALL failed"); } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return VSOCK_VQ_MAX; }
Hi, On Sun, Mar 27, 2022 at 11:45:13PM +0300, Martin Radev wrote: > > Thanks for the review. > Comments are inline. > Here is the patch: Would you mind making a new version of the series to send this patch? Thanks, Alex
diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h index 3880e74..ad274ac 100644 --- a/include/kvm/virtio.h +++ b/include/kvm/virtio.h @@ -187,7 +187,7 @@ struct virtio_ops { size_t (*get_config_size)(struct kvm *kvm, void *dev); u32 (*get_host_features)(struct kvm *kvm, void *dev); void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); - int (*get_vq_count)(struct kvm *kvm, void *dev); + unsigned int (*get_vq_count)(struct kvm *kvm, void *dev); int (*init_vq)(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align, u32 pfn); void (*exit_vq)(struct kvm *kvm, void *dev, u32 vq); diff --git a/virtio/9p.c b/virtio/9p.c index 6074f3a..7374f1e 100644 --- a/virtio/9p.c +++ b/virtio/9p.c @@ -1469,7 +1469,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/balloon.c b/virtio/balloon.c index 5bcd6ab..450b36a 100644 --- a/virtio/balloon.c +++ b/virtio/balloon.c @@ -251,7 +251,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/blk.c b/virtio/blk.c index af71c0c..46ee028 100644 --- a/virtio/blk.c +++ b/virtio/blk.c @@ -291,7 +291,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/console.c b/virtio/console.c index dae6034..8315808 100644 --- a/virtio/console.c +++ b/virtio/console.c @@ -216,7 +216,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return VIRTIO_CONSOLE_NUM_QUEUES; } diff --git a/virtio/mmio.c b/virtio/mmio.c index 0094856..d3555b4 100644 --- a/virtio/mmio.c +++ b/virtio/mmio.c @@ -175,13 +175,22 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, { struct virtio_mmio *vmmio = vdev->virtio; struct kvm *kvm = vmmio->kvm; + unsigned int vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); u32 val = 0; switch (addr) { case VIRTIO_MMIO_HOST_FEATURES_SEL: case VIRTIO_MMIO_GUEST_FEATURES_SEL: + val = ioport__read32(data); + *(u32 *)(((void *)&vmmio->hdr) + addr) = val; + break; case VIRTIO_MMIO_QUEUE_SEL: val = ioport__read32(data); + if (val >= vq_count) { + WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n", + val, vq_count); + break; + } *(u32 *)(((void *)&vmmio->hdr) + addr) = val; break; case VIRTIO_MMIO_STATUS: @@ -227,6 +236,11 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, break; case VIRTIO_MMIO_QUEUE_NOTIFY: val = ioport__read32(data); + if (val >= vq_count) { + WARN_ONCE(1, "QUEUE_NOTIFY value (%u) is larger than VQ count (%u)\n", + val, vq_count); + break; + } vdev->ops->notify_vq(vmmio->kvm, vmmio->dev, val); break; case VIRTIO_MMIO_INTERRUPT_ACK: @@ -346,10 +360,12 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev, int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev) { - int vq; + unsigned int vq; struct virtio_mmio *vmmio = vdev->virtio; + unsigned int vq_count; - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++) + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); + for (vq = 0; vq < vq_count; vq++) virtio_mmio_exit_vq(kvm, vdev, vq); return 0; diff --git a/virtio/net.c b/virtio/net.c index ec5dc1f..8dd523f 100644 --- a/virtio/net.c +++ b/virtio/net.c @@ -755,11 +755,11 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { struct net_dev *ndev = dev; - return ndev->queue_pairs * 2 + 1; + return ndev->queue_pairs * 2U + 1U; } static struct virtio_ops net_dev_virtio_ops = { diff --git a/virtio/pci.c b/virtio/pci.c index 0b5cccd..9a6cbf3 100644 --- a/virtio/pci.c +++ b/virtio/pci.c @@ -329,9 +329,11 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde struct virtio_pci *vpci; struct kvm *kvm; u32 val; + unsigned int vq_count; kvm = vcpu->kvm; vpci = vdev->virtio; + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); switch (offset) { case VIRTIO_PCI_GUEST_FEATURES: @@ -351,10 +353,21 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde } break; case VIRTIO_PCI_QUEUE_SEL: - vpci->queue_selector = ioport__read16(data); + val = ioport__read16(data); + if (val >= vq_count) { + WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n", + val, vq_count); + return false; + } + vpci->queue_selector = val; break; case VIRTIO_PCI_QUEUE_NOTIFY: val = ioport__read16(data); + if (val >= vq_count) { + WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n", + val, vq_count); + return false; + } vdev->ops->notify_vq(kvm, vpci->dev, val); break; case VIRTIO_PCI_STATUS: @@ -647,10 +660,12 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev, int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev) { - int vq; + unsigned int vq; + unsigned int vq_count; struct virtio_pci *vpci = vdev->virtio; - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++) + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); + for (vq = 0; vq < vq_count; vq++) virtio_pci_exit_vq(kvm, vdev, vq); return 0; diff --git a/virtio/rng.c b/virtio/rng.c index c7835a0..75b682e 100644 --- a/virtio/rng.c +++ b/virtio/rng.c @@ -147,7 +147,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/scsi.c b/virtio/scsi.c index 8f1c348..60432cc 100644 --- a/virtio/scsi.c +++ b/virtio/scsi.c @@ -176,7 +176,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/vsock.c b/virtio/vsock.c index 34397b6..64b4e95 100644 --- a/virtio/vsock.c +++ b/virtio/vsock.c @@ -204,7 +204,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi) die_perror("VHOST_SET_VRING_CALL failed"); } -static int get_vq_count(struct kvm *kvm, void *dev) +static unsigned int get_vq_count(struct kvm *kvm, void *dev) { return VSOCK_VQ_MAX; }
This patch checks for overflows in QUEUE_NOTIFY and QUEUE_SEL in the PCI and MMIO operation handling paths. Further, the return value type of get_vq_count is changed from int to uint since negative doesn't carry any semantic meaning. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- include/kvm/virtio.h | 2 +- virtio/9p.c | 2 +- virtio/balloon.c | 2 +- virtio/blk.c | 2 +- virtio/console.c | 2 +- virtio/mmio.c | 20 ++++++++++++++++++-- virtio/net.c | 4 ++-- virtio/pci.c | 21 ++++++++++++++++++--- virtio/rng.c | 2 +- virtio/scsi.c | 2 +- virtio/vsock.c | 2 +- 11 files changed, 46 insertions(+), 15 deletions(-)