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@xxxxxxxxx>
---
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;
}
--
2.25.1
On Wed, Mar 16, 2022 at 03:38:19PM +0000, Alexandru Elisei wrote:
> 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@xxxxxxxxx>
> > ---
> > 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.
>
I thought it would be nicer to not call this function on every loop iterations.
Still, I removed it as you suggested.
> > 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).
Done.
>
> 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
> >
