All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Martin Radev <martin.b.radev@gmail.com>
Cc: kvm@vger.kernel.org, will@kernel.org,
	julien.thierry.kdev@gmail.com, andre.przywara@arm.com
Subject: Re: [PATCH kvmtool 3/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL
Date: Wed, 16 Mar 2022 15:38:19 +0000	[thread overview]
Message-ID: <YjIEa+t4zJYMJmvB@monolith.localdoman> (raw)
In-Reply-To: <20220303231050.2146621-4-martin.b.radev@gmail.com>

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
> 

  reply	other threads:[~2022-03-16 15:38 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-03 23:10 [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Martin Radev
2022-03-03 23:10 ` [PATCH kvmtool 1/5] kvmtool: Add WARN_ONCE macro Martin Radev
2022-03-03 23:10 ` [PATCH kvmtool 2/5] virtio: Sanitize config accesses Martin Radev
2022-03-16 13:04   ` Alexandru Elisei
2022-03-27 20:37     ` Martin Radev
2022-04-22 10:12       ` Alexandru Elisei
2022-03-03 23:10 ` [PATCH kvmtool 3/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev
2022-03-16 15:38   ` Alexandru Elisei [this message]
2022-03-27 20:45     ` Martin Radev
2022-04-22 10:35       ` Alexandru Elisei
2022-03-03 23:10 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev
2022-03-03 23:10 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev
2022-03-16 15:39   ` Alexandru Elisei
2022-03-27 21:00     ` Martin Radev
2022-04-22 10:36       ` Alexandru Elisei
2022-03-10 14:56 ` [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Alexandru Elisei
2022-03-11 11:23   ` Andre Przywara
2022-03-14 17:11     ` Alexandru Elisei
2022-03-27 12:46       ` Martin Radev
2022-04-22 10:37         ` Alexandru Elisei
2022-05-06 13:20 ` Will Deacon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YjIEa+t4zJYMJmvB@monolith.localdoman \
    --to=alexandru.elisei@arm.com \
    --cc=andre.przywara@arm.com \
    --cc=julien.thierry.kdev@gmail.com \
    --cc=kvm@vger.kernel.org \
    --cc=martin.b.radev@gmail.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.