On Tue, 18 Jan 2022 00:12:00 +0200
Martin Radev <martin.b.radev@xxxxxxxxx> wrote:
Hi,
> 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 u32 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 | 25 ++++++++++++++++++++++---
> virtio/net.c | 2 +-
> virtio/pci.c | 21 ++++++++++++++++++---
> virtio/rng.c | 2 +-
> virtio/scsi.c | 2 +-
> virtio/vsock.c | 2 +-
> 11 files changed, 49 insertions(+), 15 deletions(-)
>
> diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
> index 3880e74..40f2a6d 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);
> + u32 (*get_vq_count)(struct kvm *kvm, void *dev);
I am not too happy about using an explicitly sized type for something that
is not hardware related (should be just unsigned int), but it makes
some sense below when we actually use the function.
> 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 89bec5e..8f1fc1f 100644
> --- a/virtio/9p.c
> +++ b/virtio/9p.c
> @@ -1468,7 +1468,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 u32 get_vq_count(struct kvm *kvm, void *dev)
> {
> return NUM_VIRT_QUEUES;
> }
> diff --git a/virtio/balloon.c b/virtio/balloon.c
> index 233a3a5..de3882e 100644
> --- a/virtio/balloon.c
> +++ b/virtio/balloon.c
> @@ -249,7 +249,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 u32 get_vq_count(struct kvm *kvm, void *dev)
> {
> return NUM_VIRT_QUEUES;
> }
> diff --git a/virtio/blk.c b/virtio/blk.c
> index 9164b51..46918a4 100644
> --- a/virtio/blk.c
> +++ b/virtio/blk.c
> @@ -289,7 +289,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 u32 get_vq_count(struct kvm *kvm, void *dev)
> {
> return NUM_VIRT_QUEUES;
> }
> diff --git a/virtio/console.c b/virtio/console.c
> index 00bafa2..84466d0 100644
> --- a/virtio/console.c
> +++ b/virtio/console.c
> @@ -214,7 +214,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 u32 get_vq_count(struct kvm *kvm, void *dev)
> {
> return VIRTIO_CONSOLE_NUM_QUEUES;
> }
> diff --git a/virtio/mmio.c b/virtio/mmio.c
> index 32bba17..fd9a411 100644
> --- a/virtio/mmio.c
> +++ b/virtio/mmio.c
> @@ -171,14 +171,26 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu,
> struct virtio_mmio *vmmio = vdev->virtio;
> struct kvm *kvm = vmmio->kvm;
> u32 val = 0;
> + u32 vq_count = 0;
> + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev);
This should be on one line.
>
> switch (addr) {
> case VIRTIO_MMIO_HOST_FEATURES_SEL:
> case VIRTIO_MMIO_GUEST_FEATURES_SEL:
> - case VIRTIO_MMIO_QUEUE_SEL:
> val = ioport__read32(data);
> *(u32 *)(((void *)&vmmio->hdr) + addr) = val;
> break;
> + case VIRTIO_MMIO_QUEUE_SEL:
> + {
Why the brackets here?
> + val = ioport__read32(data);
> + if (val >= vq_count) {
> + pr_warning("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:
> vmmio->hdr.status = ioport__read32(data);
> if (!vmmio->hdr.status) /* Sample endianness on reset */
> @@ -222,6 +234,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) {
Shouldn't this be ">=" here?
> + pr_warning("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:
> @@ -341,10 +358,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;
> + u32 vq;
> struct virtio_mmio *vmmio = vdev->virtio;
> + u32 vq_count;
Why this change (and the lines below)? Since get_vq_count() returns an u32
now, we should be safe?
>
> - 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 75d9ae5..9a25bfa 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -753,7 +753,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 u32 get_vq_count(struct kvm *kvm, void *dev)
> {
> struct net_dev *ndev = dev;
>
> diff --git a/virtio/pci.c b/virtio/pci.c
> index 50fdaa4..60ae2cb 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -308,9 +308,11 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde
> struct virtio_pci *vpci;
> struct kvm *kvm;
> u32 val;
> + u32 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:
> @@ -330,10 +332,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) {
> + pr_warning("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) {
> + pr_warning("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:
> @@ -626,10 +639,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;
> + u32 vq;
> struct virtio_pci *vpci = vdev->virtio;
> + u32 vq_count;
Same here, looks like an unnecessary change, since vq_count is only used
once below.
Other than that it looks good in general.
Cheers,
Andre
>
> - 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..d9b9e68 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 u32 get_vq_count(struct kvm *kvm, void *dev)
> {
> return NUM_VIRT_QUEUES;
> }
> diff --git a/virtio/scsi.c b/virtio/scsi.c
> index 37418f8..cdf553d 100644
> --- a/virtio/scsi.c
> +++ b/virtio/scsi.c
> @@ -174,7 +174,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 u32 get_vq_count(struct kvm *kvm, void *dev)
> {
> return NUM_VIRT_QUEUES;
> }
> diff --git a/virtio/vsock.c b/virtio/vsock.c
> index 2df04d7..7d523df 100644
> --- a/virtio/vsock.c
> +++ b/virtio/vsock.c
> @@ -202,7 +202,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 u32 get_vq_count(struct kvm *kvm, void *dev)
> {
> return VSOCK_VQ_MAX;
> }
