From: Andre Przywara <andre.przywara@arm.com>
To: Martin Radev <martin.b.radev@gmail.com>
Cc: kvm@vger.kernel.org, will@kernel.org,
julien.thierry.kdev@gmail.com,
Jean-Philippe Brucker <jean-philippe@linaro.org>,
Marc Zyngier <maz@kernel.org>,
Alexandru Elisei <Alexandru.Elisei@arm.com>
Subject: Re: [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL
Date: Tue, 1 Feb 2022 14:57:25 +0000 [thread overview]
Message-ID: <20220201145725.2511ef07@donnerap.cambridge.arm.com> (raw)
In-Reply-To: <bd5048ca2de1c548fa599d12fea0fa21397688af.1642457047.git.martin.b.radev@gmail.com>
On Tue, 18 Jan 2022 00:12:00 +0200
Martin Radev <martin.b.radev@gmail.com> 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@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 | 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;
> }
next prev parent reply other threads:[~2022-02-01 15:53 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-17 22:11 [PATCH kvmtool 0/5] kvmtool: Fix few found bugs Martin Radev
2022-01-17 22:11 ` [PATCH kvmtool 1/5] virtio: Sanitize config accesses Martin Radev
2022-02-01 14:55 ` Andre Przywara
2022-02-01 15:27 ` Alexandru Elisei
2022-01-17 22:12 ` [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev
2022-02-01 14:57 ` Andre Przywara [this message]
2022-02-01 15:28 ` Alexandru Elisei
2022-01-17 22:12 ` [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled Martin Radev
2022-02-01 14:57 ` Andre Przywara
2022-02-01 15:31 ` Alexandru Elisei
2022-01-17 22:12 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev
2022-02-01 15:01 ` Andre Przywara
2022-02-01 15:33 ` Alexandru Elisei
2022-01-17 22:12 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev
2022-02-01 15:34 ` Alexandru Elisei
2022-02-01 15:52 ` Andre Przywara
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=20220201145725.2511ef07@donnerap.cambridge.arm.com \
--to=andre.przywara@arm.com \
--cc=Alexandru.Elisei@arm.com \
--cc=jean-philippe@linaro.org \
--cc=julien.thierry.kdev@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=martin.b.radev@gmail.com \
--cc=maz@kernel.org \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox