[PATCH kvmtool 0/5] kvmtool: Fix few found bugs

[PATCH kvmtool 0/5] kvmtool: Fix few found bugs

[Martin Radev]

In December, we hosted a CTF where one of the challenges was exploiting
any "0day" bug in kvmtool [1]. Eight teams managed to find a bug and
exploit it in less than 36 hours. Write-ups for exploits are available
by HXP [2] and kalmarunionen [3]. 

Now, I'm aware that kvmtool is mostly used for KVM testing and KVM bring-up
in simulation environments. But since it does get mentioned in some security-
related projects [4, 5] and has a sandboxing feature, maybe it makes sense
to fix these bugs.

Could you please check if these patches make sense?
I have not verified that these patches do not break something for these virtio
drivers.

Kind regards,
Martin

[1]: https://2021.ctf.link/internal/challenge/dd0e8826-c970-4fde-8eeb-41a9d8a86b67/
[2]: https://hxp.io/blog/87/hxp-CTF-2021-indie_vmm-writeup/
[3]: https://www.kalmarunionen.dk/writeups/2021/hxp-2021/lkvm/
[4]: https://blog.quarkslab.com/no-tears-no-fears.html
[5]: https://fly.io/blog/sandboxing-and-workload-isolation/

Martin Radev (5):
  virtio: Sanitize config accesses
  virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL
  virtio/net: Warn if virtio_net is implicitly enabled
  Makefile: Mark stack as not executable
  mmio: Sanitize addr and len

 Makefile                |  7 +++++--
 include/kvm/virtio-9p.h |  1 +
 include/kvm/virtio.h    |  3 ++-
 mmio.c                  |  4 ++++
 virtio/9p.c             | 21 ++++++++++++++++----
 virtio/balloon.c        |  8 +++++++-
 virtio/blk.c            |  8 +++++++-
 virtio/console.c        |  8 +++++++-
 virtio/mmio.c           | 44 ++++++++++++++++++++++++++++++++++-------
 virtio/net.c            | 11 ++++++++++-
 virtio/pci.c            | 40 +++++++++++++++++++++++++++++++++----
 virtio/rng.c            |  8 +++++++-
 virtio/scsi.c           |  8 +++++++-
 virtio/vsock.c          |  8 +++++++-
 14 files changed, 154 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH kvmtool 1/5] virtio: Sanitize config accesses

[Martin Radev]

The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows.
This patch sanitizes this operation by using the newly added virtio op
get_config_size. Any access which goes beyond the config structure's
size is prevented and a failure is returned.

Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
 include/kvm/virtio-9p.h |  1 +
 include/kvm/virtio.h    |  1 +
 virtio/9p.c             | 19 ++++++++++++++++---
 virtio/balloon.c        |  6 ++++++
 virtio/blk.c            |  6 ++++++
 virtio/console.c        |  6 ++++++
 virtio/mmio.c           | 19 +++++++++++++++----
 virtio/net.c            |  6 ++++++
 virtio/pci.c            | 19 ++++++++++++++++++-
 virtio/rng.c            |  6 ++++++
 virtio/scsi.c           |  6 ++++++
 virtio/vsock.c          |  6 ++++++
 12 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h
index 3ea7698..77c5062 100644
--- a/include/kvm/virtio-9p.h
+++ b/include/kvm/virtio-9p.h
@@ -44,6 +44,7 @@ struct p9_dev {
 	struct virtio_device	vdev;
 	struct rb_root		fids;
 
+	size_t config_size;
 	struct virtio_9p_config	*config;
 	u32			features;
 
diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
index 3a311f5..3880e74 100644
--- a/include/kvm/virtio.h
+++ b/include/kvm/virtio.h
@@ -184,6 +184,7 @@ struct virtio_device {
 
 struct virtio_ops {
 	u8 *(*get_config)(struct kvm *kvm, void *dev);
+	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);
diff --git a/virtio/9p.c b/virtio/9p.c
index b78f2b3..89bec5e 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -1375,6 +1375,12 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(p9dev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	struct p9_dev *p9dev = dev;
+	return p9dev->config_size;
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	return 1 << VIRTIO_9P_MOUNT_TAG;
@@ -1469,6 +1475,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
 
 struct virtio_ops p9_dev_virtio_ops = {
 	.get_config		= get_config,
+	.get_config_size	= get_config_size,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
 	.init_vq		= init_vq,
@@ -1569,6 +1576,8 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
 {
 	struct p9_dev *p9dev;
 	int err = 0;
+	size_t tag_name_length = 0;
+	size_t config_size = 0;
 
 	p9dev = calloc(1, sizeof(*p9dev));
 	if (!p9dev)
@@ -1577,22 +1586,26 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
 	if (!tag_name)
 		tag_name = VIRTIO_9P_DEFAULT_TAG;
 
-	p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1);
+	tag_name_length = strlen(tag_name);
+	config_size = sizeof(*p9dev->config) + tag_name_length + 1;
+
+	p9dev->config = calloc(1, config_size);
 	if (p9dev->config == NULL) {
 		err = -ENOMEM;
 		goto free_p9dev;
 	}
+	p9dev->config_size = config_size;
 
 	strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir));
 	p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00';
 
-	p9dev->config->tag_len = strlen(tag_name);
+	p9dev->config->tag_len = tag_name_length;
 	if (p9dev->config->tag_len > MAX_TAG_LEN) {
 		err = -EINVAL;
 		goto free_p9dev_config;
 	}
 
-	memcpy(&p9dev->config->tag, tag_name, strlen(tag_name));
+	memcpy(&p9dev->config->tag, tag_name, tag_name_length);
 
 	list_add(&p9dev->list, &devs);
 
diff --git a/virtio/balloon.c b/virtio/balloon.c
index 8e8803f..233a3a5 100644
--- a/virtio/balloon.c
+++ b/virtio/balloon.c
@@ -181,6 +181,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&bdev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	return sizeof(struct virtio_balloon_config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	return 1 << VIRTIO_BALLOON_F_STATS_VQ;
@@ -251,6 +256,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
 
 struct virtio_ops bln_dev_virtio_ops = {
 	.get_config		= get_config,
+	.get_config_size	= get_config_size,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
 	.init_vq		= init_vq,
diff --git a/virtio/blk.c b/virtio/blk.c
index 4d02d10..9164b51 100644
--- a/virtio/blk.c
+++ b/virtio/blk.c
@@ -146,6 +146,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&bdev->blk_config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	return sizeof(struct virtio_blk_config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	struct blk_dev *bdev = dev;
@@ -291,6 +296,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
 
 static struct virtio_ops blk_dev_virtio_ops = {
 	.get_config		= get_config,
+	.get_config_size	= get_config_size,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
 	.get_vq_count		= get_vq_count,
diff --git a/virtio/console.c b/virtio/console.c
index e0b98df..00bafa2 100644
--- a/virtio/console.c
+++ b/virtio/console.c
@@ -121,6 +121,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&cdev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	return sizeof(struct virtio_console_config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	return 0;
@@ -216,6 +221,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
 
 static struct virtio_ops con_dev_virtio_ops = {
 	.get_config		= get_config,
+	.get_config_size	= get_config_size,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
 	.get_vq_count		= get_vq_count,
diff --git a/virtio/mmio.c b/virtio/mmio.c
index 875a288..32bba17 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -103,15 +103,26 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu,
 					u8 is_write, struct virtio_device *vdev)
 {
 	struct virtio_mmio *vmmio = vdev->virtio;
+	u8 *config_aperture = NULL;
+	u32 config_aperture_size = 0;
 	u32 i;
 
+	config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev);
+	/* The cast here is safe because get_config_size will always fit in 32 bits. */
+	config_aperture_size = (u32)vdev->ops->get_config_size(vmmio->kvm, vmmio->dev);
+
+	/* Reduce length to no more than the config size to avoid buffer overflows. */
+	if (config_aperture_size < len) {
+		pr_warning("Length (%u) goes beyond config size (%u).\n",
+			len, config_aperture_size);
+		len = config_aperture_size;
+	}
+
 	for (i = 0; i < len; i++) {
 		if (is_write)
-			vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] =
-					      *(u8 *)data + i;
+			config_aperture[addr + i] = *(u8 *)data + i;
 		else
-			data[i] = vdev->ops->get_config(vmmio->kvm,
-							vmmio->dev)[addr + i];
+			data[i] = config_aperture[addr+i];
 	}
 }
 
diff --git a/virtio/net.c b/virtio/net.c
index 1ee3c19..75d9ae5 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -480,6 +480,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&ndev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	return sizeof(struct virtio_net_config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	u32 features;
@@ -757,6 +762,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
 
 static struct virtio_ops net_dev_virtio_ops = {
 	.get_config		= get_config,
+	.get_config_size	= get_config_size,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
 	.get_vq_count		= get_vq_count,
diff --git a/virtio/pci.c b/virtio/pci.c
index 4108529..50fdaa4 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -136,7 +136,15 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device *
 		return true;
 	} else if (type == VIRTIO_PCI_O_CONFIG) {
 		u8 cfg;
-
+		size_t config_size;
+
+		config_size = vdev->ops->get_config_size(kvm, vpci->dev);
+		if (config_offset >= config_size) {
+			/* Access goes beyond the config size, so return failure. */
+			pr_warning("Config access offset (%u) is beyond config size (%zu)\n",
+				config_offset, config_size);
+			return false;
+		}
 		cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset];
 		ioport__write8(data, cfg);
 		return true;
@@ -276,6 +284,15 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device
 
 		return true;
 	} else if (type == VIRTIO_PCI_O_CONFIG) {
+		size_t config_size;
+
+		config_size = vdev->ops->get_config_size(kvm, vpci->dev);
+		if (config_offset >= config_size) {
+			/* Access goes beyond the config size, so return failure. */
+			pr_warning("Config access offset (%u) is beyond config size (%zu)\n",
+				config_offset, config_size);
+			return false;
+		}
 		vdev->ops->get_config(kvm, vpci->dev)[config_offset] = *(u8 *)data;
 
 		return true;
diff --git a/virtio/rng.c b/virtio/rng.c
index 78eaa64..c7835a0 100644
--- a/virtio/rng.c
+++ b/virtio/rng.c
@@ -47,6 +47,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return 0;
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	return 0;
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	/* Unused */
@@ -149,6 +154,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
 
 static struct virtio_ops rng_dev_virtio_ops = {
 	.get_config		= get_config,
+	.get_config_size	= get_config_size,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
 	.init_vq		= init_vq,
diff --git a/virtio/scsi.c b/virtio/scsi.c
index 16a86cb..37418f8 100644
--- a/virtio/scsi.c
+++ b/virtio/scsi.c
@@ -38,6 +38,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&sdev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	return sizeof(struct virtio_scsi_config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	return	1UL << VIRTIO_RING_F_EVENT_IDX |
@@ -176,6 +181,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
 
 static struct virtio_ops scsi_dev_virtio_ops = {
 	.get_config		= get_config,
+	.get_config_size	= get_config_size,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
 	.init_vq		= init_vq,
diff --git a/virtio/vsock.c b/virtio/vsock.c
index 5b99838..2df04d7 100644
--- a/virtio/vsock.c
+++ b/virtio/vsock.c
@@ -41,6 +41,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&vdev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	return sizeof(struct virtio_vsock_config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	return 1UL << VIRTIO_RING_F_EVENT_IDX
@@ -204,6 +209,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
 
 static struct virtio_ops vsock_dev_virtio_ops = {
 	.get_config		= get_config,
+	.get_config_size	= get_config_size,
 	.get_host_features	= get_host_features,
 	.set_guest_features	= set_guest_features,
 	.init_vq		= init_vq,
-- 
2.25.1

[PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL

[Martin Radev]

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);
 	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);
 
 	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:
+		{
+			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) {
+			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;
 
-	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;
 
-	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;
 }
-- 
2.25.1

[PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled

[Martin Radev]

The virtio_net device is implicitly enabled if user doesn't
explicitly specify that virtio_net is disabled. This is
counter-intuitive to how the rest of the virtio commandline
works.

For backwards-compatibility, the commandline parameters are
not changed. Instead, this patch prints out a warning if
virtio_net is implicitly enabled.

Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
 virtio/net.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virtio/net.c b/virtio/net.c
index 9a25bfa..ab75d40 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -1002,6 +1002,9 @@ int virtio_net__init(struct kvm *kvm)
 
 	if (kvm->cfg.num_net_devices == 0 && kvm->cfg.no_net == 0) {
 		static struct virtio_net_params net_params;
+		pr_warning(
+			"No net devices configured, but no_net not specified. "
+			"Enabling virtio_net with default network settings...\n");
 
 		net_params = (struct virtio_net_params) {
 			.guest_ip	= kvm->cfg.guest_ip,
-- 
2.25.1

[PATCH kvmtool 4/5] Makefile: Mark stack as not executable

[Martin Radev]

This patch modifies CFLAGS to mark the stack explicitly
as not executable.

Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
 Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index f251147..09ef282 100644
--- a/Makefile
+++ b/Makefile
@@ -380,8 +380,11 @@ DEFINES	+= -D_GNU_SOURCE
 DEFINES	+= -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"'
 DEFINES	+= -DBUILD_ARCH='"$(ARCH)"'
 
+# The stack doesn't need to be executable
+SECURITY_HARDENINGS := -z noexecstack
+
 KVM_INCLUDE := include
-CFLAGS	+= $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
+CFLAGS	+= $(CPPFLAGS) $(DEFINES) $(SECURITY_HARDENINGS) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
 
 WARNINGS += -Wall
 WARNINGS += -Wformat=2
@@ -582,4 +585,4 @@ ifneq ($(MAKECMDGOALS),clean)
 
 KVMTOOLS-VERSION-FILE:
 	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
-endif
\ No newline at end of file
+endif
-- 
2.25.1

[PATCH kvmtool 5/5] mmio: Sanitize addr and len

[Martin Radev]

This patch verifies that adding the addr and length arguments
from an MMIO op do not overflow. This is necessary because the
arguments are controlled by the VM. The length may be set to
an arbitrary value by using the rep prefix.

Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
 mmio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mmio.c b/mmio.c
index a6dd3aa..04d2af6 100644
--- a/mmio.c
+++ b/mmio.c
@@ -32,6 +32,10 @@ static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len)
 {
 	struct rb_int_node *node;
 
+	/* If len is zero or if there's an overflow, the MMIO op is invalid. */
+	if (len + addr <= addr)
+		return NULL;
+
 	node = rb_int_search_range(root, addr, addr + len);
 	if (node == NULL)
 		return NULL;
-- 
2.25.1

Re: [PATCH kvmtool 1/5] virtio: Sanitize config accesses

[Andre Przywara]

On Tue, 18 Jan 2022 00:11:59 +0200
Martin Radev <martin.b.radev@gmail.com> wrote:

Hi Martin,

many thanks for sending this!

In general that looks good to me, just some minor things below.

> The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows.
> This patch sanitizes this operation by using the newly added virtio op
> get_config_size. Any access which goes beyond the config structure's
> size is prevented and a failure is returned.
> 
> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
> ---
>  include/kvm/virtio-9p.h |  1 +
>  include/kvm/virtio.h    |  1 +
>  virtio/9p.c             | 19 ++++++++++++++++---
>  virtio/balloon.c        |  6 ++++++
>  virtio/blk.c            |  6 ++++++
>  virtio/console.c        |  6 ++++++
>  virtio/mmio.c           | 19 +++++++++++++++----
>  virtio/net.c            |  6 ++++++
>  virtio/pci.c            | 19 ++++++++++++++++++-
>  virtio/rng.c            |  6 ++++++
>  virtio/scsi.c           |  6 ++++++
>  virtio/vsock.c          |  6 ++++++
>  12 files changed, 93 insertions(+), 8 deletions(-)
> 
> diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h
> index 3ea7698..77c5062 100644
> --- a/include/kvm/virtio-9p.h
> +++ b/include/kvm/virtio-9p.h
> @@ -44,6 +44,7 @@ struct p9_dev {
>  	struct virtio_device	vdev;
>  	struct rb_root		fids;
>  
> +	size_t config_size;
>  	struct virtio_9p_config	*config;
>  	u32			features;
>  
> diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
> index 3a311f5..3880e74 100644
> --- a/include/kvm/virtio.h
> +++ b/include/kvm/virtio.h
> @@ -184,6 +184,7 @@ struct virtio_device {
>  
>  struct virtio_ops {
>  	u8 *(*get_config)(struct kvm *kvm, void *dev);
> +	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);
> diff --git a/virtio/9p.c b/virtio/9p.c
> index b78f2b3..89bec5e 100644
> --- a/virtio/9p.c
> +++ b/virtio/9p.c
> @@ -1375,6 +1375,12 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(p9dev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	struct p9_dev *p9dev = dev;

I think it's customary to have an empty line after variable declarations.

> +	return p9dev->config_size;
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return 1 << VIRTIO_9P_MOUNT_TAG;
> @@ -1469,6 +1475,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  struct virtio_ops p9_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,
> @@ -1569,6 +1576,8 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
>  {
>  	struct p9_dev *p9dev;
>  	int err = 0;
> +	size_t tag_name_length = 0;
> +	size_t config_size = 0;

You should not need to initialise them here, as they are unconditionally
set by the code below. Any premature use would then be spotted by the
compiler.

>  
>  	p9dev = calloc(1, sizeof(*p9dev));
>  	if (!p9dev)
> @@ -1577,22 +1586,26 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
>  	if (!tag_name)
>  		tag_name = VIRTIO_9P_DEFAULT_TAG;
>  
> -	p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1);
> +	tag_name_length = strlen(tag_name);
> +	config_size = sizeof(*p9dev->config) + tag_name_length + 1;
> +
> +	p9dev->config = calloc(1, config_size);
>  	if (p9dev->config == NULL) {
>  		err = -ENOMEM;
>  		goto free_p9dev;
>  	}
> +	p9dev->config_size = config_size;
>  
>  	strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir));
>  	p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00';
>  
> -	p9dev->config->tag_len = strlen(tag_name);
> +	p9dev->config->tag_len = tag_name_length;
>  	if (p9dev->config->tag_len > MAX_TAG_LEN) {
>  		err = -EINVAL;
>  		goto free_p9dev_config;
>  	}
>  
> -	memcpy(&p9dev->config->tag, tag_name, strlen(tag_name));
> +	memcpy(&p9dev->config->tag, tag_name, tag_name_length);
>  
>  	list_add(&p9dev->list, &devs);
>  
> diff --git a/virtio/balloon.c b/virtio/balloon.c
> index 8e8803f..233a3a5 100644
> --- a/virtio/balloon.c
> +++ b/virtio/balloon.c
> @@ -181,6 +181,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&bdev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_balloon_config);

I wonder if we should return the size of the variable instead of the type,
which is more future proof:
        struct bln_dev *bdev = dev;

        return sizeof bln_dev->config;

(same for all the other occasions below)

> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return 1 << VIRTIO_BALLOON_F_STATS_VQ;
> @@ -251,6 +256,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  struct virtio_ops bln_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,
> diff --git a/virtio/blk.c b/virtio/blk.c
> index 4d02d10..9164b51 100644
> --- a/virtio/blk.c
> +++ b/virtio/blk.c
> @@ -146,6 +146,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&bdev->blk_config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_blk_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	struct blk_dev *bdev = dev;
> @@ -291,6 +296,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops blk_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.get_vq_count		= get_vq_count,
> diff --git a/virtio/console.c b/virtio/console.c
> index e0b98df..00bafa2 100644
> --- a/virtio/console.c
> +++ b/virtio/console.c
> @@ -121,6 +121,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&cdev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_console_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return 0;
> @@ -216,6 +221,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops con_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.get_vq_count		= get_vq_count,
> diff --git a/virtio/mmio.c b/virtio/mmio.c
> index 875a288..32bba17 100644
> --- a/virtio/mmio.c
> +++ b/virtio/mmio.c
> @@ -103,15 +103,26 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu,
>  					u8 is_write, struct virtio_device *vdev)
>  {
>  	struct virtio_mmio *vmmio = vdev->virtio;
> +	u8 *config_aperture = NULL;
> +	u32 config_aperture_size = 0;

Why is this a u32?
And again, no need to initialise here, actually you can pull in the next
two lines to initialise them properly right away.

>  	u32 i;
>  
> +	config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev);
> +	/* The cast here is safe because get_config_size will always fit in 32 bits. */
> +	config_aperture_size = (u32)vdev->ops->get_config_size(vmmio->kvm, vmmio->dev);

Why the cast in the first place? Can't you just leave the variable at
size_t, and let the compiler deal with the promotion below?

> +
> +	/* Reduce length to no more than the config size to avoid buffer overflows. */
> +	if (config_aperture_size < len) {
> +		pr_warning("Length (%u) goes beyond config size (%u).\n",
> +			len, config_aperture_size);

So I appreciate the warning, but I wonder if this is too much, or should
be rate limited, otherwise we allow the guest to flood the VMM's terminal.

Is there any good practices for this problem? For kernel printk's we
definitely don't allow this.

 +		len = config_aperture_size;

This should be safe in any case (even with different variable sizes), since
you check that above. Or am I missing something?

> +	}
> +
>  	for (i = 0; i < len; i++) {
>  		if (is_write)
> -			vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] =
> -					      *(u8 *)data + i;
> +			config_aperture[addr + i] = *(u8 *)data + i;
>  		else
> -			data[i] = vdev->ops->get_config(vmmio->kvm,
> -							vmmio->dev)[addr + i];
> +			data[i] = config_aperture[addr+i];

Nit:                                              addr + i

>  	}
>  }
>  
> diff --git a/virtio/net.c b/virtio/net.c
> index 1ee3c19..75d9ae5 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -480,6 +480,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&ndev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_net_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	u32 features;
> @@ -757,6 +762,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops net_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.get_vq_count		= get_vq_count,
> diff --git a/virtio/pci.c b/virtio/pci.c
> index 4108529..50fdaa4 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -136,7 +136,15 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device *
>  		return true;
>  	} else if (type == VIRTIO_PCI_O_CONFIG) {
>  		u8 cfg;
> -
> +		size_t config_size;
> +
> +		config_size = vdev->ops->get_config_size(kvm, vpci->dev);
> +		if (config_offset >= config_size) {
> +			/* Access goes beyond the config size, so return failure. */
> +			pr_warning("Config access offset (%u) is beyond config size (%zu)\n",
> +				config_offset, config_size);

Again, do we always want to warn on the console?
Another alternative would be to kill the guest at this point, but this is
probably too much.

Cheers,
Andre


> +			return false;
> +		}
>  		cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset];
>  		ioport__write8(data, cfg);
>  		return true;
> @@ -276,6 +284,15 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device
>  
>  		return true;
>  	} else if (type == VIRTIO_PCI_O_CONFIG) {
> +		size_t config_size;
> +
> +		config_size = vdev->ops->get_config_size(kvm, vpci->dev);
> +		if (config_offset >= config_size) {
> +			/* Access goes beyond the config size, so return failure. */
> +			pr_warning("Config access offset (%u) is beyond config size (%zu)\n",
> +				config_offset, config_size);
> +			return false;
> +		}
>  		vdev->ops->get_config(kvm, vpci->dev)[config_offset] = *(u8 *)data;
>  
>  		return true;
> diff --git a/virtio/rng.c b/virtio/rng.c
> index 78eaa64..c7835a0 100644
> --- a/virtio/rng.c
> +++ b/virtio/rng.c
> @@ -47,6 +47,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return 0;
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return 0;
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	/* Unused */
> @@ -149,6 +154,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops rng_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,
> diff --git a/virtio/scsi.c b/virtio/scsi.c
> index 16a86cb..37418f8 100644
> --- a/virtio/scsi.c
> +++ b/virtio/scsi.c
> @@ -38,6 +38,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&sdev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_scsi_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return	1UL << VIRTIO_RING_F_EVENT_IDX |
> @@ -176,6 +181,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops scsi_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,
> diff --git a/virtio/vsock.c b/virtio/vsock.c
> index 5b99838..2df04d7 100644
> --- a/virtio/vsock.c
> +++ b/virtio/vsock.c
> @@ -41,6 +41,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&vdev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_vsock_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return 1UL << VIRTIO_RING_F_EVENT_IDX
> @@ -204,6 +209,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops vsock_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,

Re: [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL

[Andre Przywara]

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;
>  }

Re: [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled

[Andre Przywara]

On Tue, 18 Jan 2022 00:12:01 +0200
Martin Radev <martin.b.radev@gmail.com> wrote:

Hi,

> The virtio_net device is implicitly enabled if user doesn't
> explicitly specify that virtio_net is disabled. This is
> counter-intuitive to how the rest of the virtio commandline
> works.
> 
> For backwards-compatibility, the commandline parameters are
> not changed. Instead, this patch prints out a warning if
> virtio_net is implicitly enabled.
> 
> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
> ---
>  virtio/net.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/virtio/net.c b/virtio/net.c
> index 9a25bfa..ab75d40 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -1002,6 +1002,9 @@ int virtio_net__init(struct kvm *kvm)
>  
>  	if (kvm->cfg.num_net_devices == 0 && kvm->cfg.no_net == 0) {
>  		static struct virtio_net_params net_params;
> +		pr_warning(
> +			"No net devices configured, but no_net not specified. "
> +			"Enabling virtio_net with default network settings...\n");

I am a bit unsure about it. We recently tried to get rid of those molly
guard messages, so I am not sure we should issue a *warning* here. After
all, nothing is wrong, it's documented behaviour.

Cheers,
Andre

>  
>  		net_params = (struct virtio_net_params) {
>  			.guest_ip	= kvm->cfg.guest_ip,

Re: [PATCH kvmtool 4/5] Makefile: Mark stack as not executable

[Andre Przywara]

On Tue, 18 Jan 2022 00:12:02 +0200
Martin Radev <martin.b.radev@gmail.com> wrote:

> This patch modifies CFLAGS to mark the stack explicitly
> as not executable.
> 
> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  Makefile | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index f251147..09ef282 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -380,8 +380,11 @@ DEFINES	+= -D_GNU_SOURCE
>  DEFINES	+= -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"'
>  DEFINES	+= -DBUILD_ARCH='"$(ARCH)"'
>  
> +# The stack doesn't need to be executable
> +SECURITY_HARDENINGS := -z noexecstack
> +
>  KVM_INCLUDE := include
> -CFLAGS	+= $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
> +CFLAGS	+= $(CPPFLAGS) $(DEFINES) $(SECURITY_HARDENINGS) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
>  
>  WARNINGS += -Wall
>  WARNINGS += -Wformat=2
> @@ -582,4 +585,4 @@ ifneq ($(MAKECMDGOALS),clean)
>  
>  KVMTOOLS-VERSION-FILE:
>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> -endif
> \ No newline at end of file
> +endif

Re: [PATCH kvmtool 1/5] virtio: Sanitize config accesses

[Alexandru Elisei]

Hi Martin,

I'm on shaky ground when it comes to the virtio spec, but I'll do my best
to review your patch.

On Tue, Jan 18, 2022 at 12:11:59AM +0200, Martin Radev wrote:
> The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows.
> This patch sanitizes this operation by using the newly added virtio op
> get_config_size. Any access which goes beyond the config structure's
> size is prevented and a failure is returned.
> 
> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
> ---
>  include/kvm/virtio-9p.h |  1 +
>  include/kvm/virtio.h    |  1 +
>  virtio/9p.c             | 19 ++++++++++++++++---
>  virtio/balloon.c        |  6 ++++++
>  virtio/blk.c            |  6 ++++++
>  virtio/console.c        |  6 ++++++
>  virtio/mmio.c           | 19 +++++++++++++++----
>  virtio/net.c            |  6 ++++++
>  virtio/pci.c            | 19 ++++++++++++++++++-
>  virtio/rng.c            |  6 ++++++
>  virtio/scsi.c           |  6 ++++++
>  virtio/vsock.c          |  6 ++++++
>  12 files changed, 93 insertions(+), 8 deletions(-)
> 
> diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h
> index 3ea7698..77c5062 100644
> --- a/include/kvm/virtio-9p.h
> +++ b/include/kvm/virtio-9p.h
> @@ -44,6 +44,7 @@ struct p9_dev {
>  	struct virtio_device	vdev;
>  	struct rb_root		fids;
>  
> +	size_t config_size;
>  	struct virtio_9p_config	*config;
>  	u32			features;
>  
> diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
> index 3a311f5..3880e74 100644
> --- a/include/kvm/virtio.h
> +++ b/include/kvm/virtio.h
> @@ -184,6 +184,7 @@ struct virtio_device {
>  
>  struct virtio_ops {
>  	u8 *(*get_config)(struct kvm *kvm, void *dev);
> +	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);
> diff --git a/virtio/9p.c b/virtio/9p.c
> index b78f2b3..89bec5e 100644
> --- a/virtio/9p.c
> +++ b/virtio/9p.c
> @@ -1375,6 +1375,12 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(p9dev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	struct p9_dev *p9dev = dev;
> +	return p9dev->config_size;
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return 1 << VIRTIO_9P_MOUNT_TAG;
> @@ -1469,6 +1475,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  struct virtio_ops p9_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,
> @@ -1569,6 +1576,8 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
>  {
>  	struct p9_dev *p9dev;
>  	int err = 0;
> +	size_t tag_name_length = 0;
> +	size_t config_size = 0;
>  
>  	p9dev = calloc(1, sizeof(*p9dev));
>  	if (!p9dev)
> @@ -1577,22 +1586,26 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
>  	if (!tag_name)
>  		tag_name = VIRTIO_9P_DEFAULT_TAG;
>  
> -	p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1);
> +	tag_name_length = strlen(tag_name);
> +	config_size = sizeof(*p9dev->config) + tag_name_length + 1;
> +
> +	p9dev->config = calloc(1, config_size);
>  	if (p9dev->config == NULL) {
>  		err = -ENOMEM;
>  		goto free_p9dev;
>  	}
> +	p9dev->config_size = config_size;
>  
>  	strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir));
>  	p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00';
>  
> -	p9dev->config->tag_len = strlen(tag_name);
> +	p9dev->config->tag_len = tag_name_length;
>  	if (p9dev->config->tag_len > MAX_TAG_LEN) {
>  		err = -EINVAL;
>  		goto free_p9dev_config;
>  	}
>  
> -	memcpy(&p9dev->config->tag, tag_name, strlen(tag_name));
> +	memcpy(&p9dev->config->tag, tag_name, tag_name_length);
>  
>  	list_add(&p9dev->list, &devs);
>  
> diff --git a/virtio/balloon.c b/virtio/balloon.c
> index 8e8803f..233a3a5 100644
> --- a/virtio/balloon.c
> +++ b/virtio/balloon.c
> @@ -181,6 +181,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&bdev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_balloon_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return 1 << VIRTIO_BALLOON_F_STATS_VQ;
> @@ -251,6 +256,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  struct virtio_ops bln_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,
> diff --git a/virtio/blk.c b/virtio/blk.c
> index 4d02d10..9164b51 100644
> --- a/virtio/blk.c
> +++ b/virtio/blk.c
> @@ -146,6 +146,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&bdev->blk_config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_blk_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	struct blk_dev *bdev = dev;
> @@ -291,6 +296,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops blk_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.get_vq_count		= get_vq_count,
> diff --git a/virtio/console.c b/virtio/console.c
> index e0b98df..00bafa2 100644
> --- a/virtio/console.c
> +++ b/virtio/console.c
> @@ -121,6 +121,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&cdev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_console_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return 0;
> @@ -216,6 +221,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops con_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.get_vq_count		= get_vq_count,
> diff --git a/virtio/mmio.c b/virtio/mmio.c
> index 875a288..32bba17 100644
> --- a/virtio/mmio.c
> +++ b/virtio/mmio.c
> @@ -103,15 +103,26 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu,
>  					u8 is_write, struct virtio_device *vdev)
>  {
>  	struct virtio_mmio *vmmio = vdev->virtio;
> +	u8 *config_aperture = NULL;
> +	u32 config_aperture_size = 0;
>  	u32 i;
>  
> +	config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev);
> +	/* The cast here is safe because get_config_size will always fit in 32 bits. */
> +	config_aperture_size = (u32)vdev->ops->get_config_size(vmmio->kvm, vmmio->dev);
> +
> +	/* Reduce length to no more than the config size to avoid buffer overflows. */
> +	if (config_aperture_size < len) {
> +		pr_warning("Length (%u) goes beyond config size (%u).\n",
> +			len, config_aperture_size);
> +		len = config_aperture_size;
> +	}

I think the case where addr is beyong the config region for the device
isn't handled (it is handled in the case of the PCI transport). I think the
function should return early if that's the case.

> +
>  	for (i = 0; i < len; i++) {
>  		if (is_write)
> -			vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] =
> -					      *(u8 *)data + i;
> +			config_aperture[addr + i] = *(u8 *)data + i;
>  		else
> -			data[i] = vdev->ops->get_config(vmmio->kvm,
> -							vmmio->dev)[addr + i];
> +			data[i] = config_aperture[addr+i];

Nitpick: it should be addr + i (notice the spaces) to be consistent with
the rest of the file.

>  	}
>  }
>  
> diff --git a/virtio/net.c b/virtio/net.c
> index 1ee3c19..75d9ae5 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -480,6 +480,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&ndev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_net_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	u32 features;
> @@ -757,6 +762,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops net_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.get_vq_count		= get_vq_count,
> diff --git a/virtio/pci.c b/virtio/pci.c
> index 4108529..50fdaa4 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -136,7 +136,15 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device *
>  		return true;
>  	} else if (type == VIRTIO_PCI_O_CONFIG) {
>  		u8 cfg;
> -
> +		size_t config_size;
> +
> +		config_size = vdev->ops->get_config_size(kvm, vpci->dev);
> +		if (config_offset >= config_size) {

What happens when offset + size > vdev->ops.get_config() + config_size?
This case is handled in the mmio transport case, but not here. More details
below.


> +			/* Access goes beyond the config size, so return failure. */
> +			pr_warning("Config access offset (%u) is beyond config size (%zu)\n",
> +				config_offset, config_size);
> +			return false;
> +		}
>  		cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset];
>  		ioport__write8(data, cfg);

I think this is wrong. Looking at struct virtio_net_config, there are
fields larger than a byte and the read here doesn't take into account the
size parameter of the function. IMO, the function should be fixed to read
the correct size before doing any boundary checks.

This is what virtio-v1.0 has to say about transitional devices:

"When using the legacy interface the driver MAY access the device-specific
configuration region using any width accesses, and a transitional device
MUST present driver with the same results as when accessed using the
“natural” access method (i.e. 32-bit accesses for 32-bit fields, etc)".

Is there something that I'm missing here?

Same thing with virtio_pci__specific_data_out() below, it ignores the size
parameter.

Thanks,
Alex

>  		return true;
> @@ -276,6 +284,15 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device
>  
>  		return true;
>  	} else if (type == VIRTIO_PCI_O_CONFIG) {
> +		size_t config_size;
> +
> +		config_size = vdev->ops->get_config_size(kvm, vpci->dev);
> +		if (config_offset >= config_size) {
> +			/* Access goes beyond the config size, so return failure. */
> +			pr_warning("Config access offset (%u) is beyond config size (%zu)\n",
> +				config_offset, config_size);
> +			return false;
> +		}
>  		vdev->ops->get_config(kvm, vpci->dev)[config_offset] = *(u8 *)data;
>  
>  		return true;
> diff --git a/virtio/rng.c b/virtio/rng.c
> index 78eaa64..c7835a0 100644
> --- a/virtio/rng.c
> +++ b/virtio/rng.c
> @@ -47,6 +47,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return 0;
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return 0;
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	/* Unused */
> @@ -149,6 +154,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops rng_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,
> diff --git a/virtio/scsi.c b/virtio/scsi.c
> index 16a86cb..37418f8 100644
> --- a/virtio/scsi.c
> +++ b/virtio/scsi.c
> @@ -38,6 +38,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&sdev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_scsi_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return	1UL << VIRTIO_RING_F_EVENT_IDX |
> @@ -176,6 +181,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops scsi_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,
> diff --git a/virtio/vsock.c b/virtio/vsock.c
> index 5b99838..2df04d7 100644
> --- a/virtio/vsock.c
> +++ b/virtio/vsock.c
> @@ -41,6 +41,11 @@ static u8 *get_config(struct kvm *kvm, void *dev)
>  	return ((u8 *)(&vdev->config));
>  }
>  
> +static size_t get_config_size(struct kvm *kvm, void *dev)
> +{
> +	return sizeof(struct virtio_vsock_config);
> +}
> +
>  static u32 get_host_features(struct kvm *kvm, void *dev)
>  {
>  	return 1UL << VIRTIO_RING_F_EVENT_IDX
> @@ -204,6 +209,7 @@ static int get_vq_count(struct kvm *kvm, void *dev)
>  
>  static struct virtio_ops vsock_dev_virtio_ops = {
>  	.get_config		= get_config,
> +	.get_config_size	= get_config_size,
>  	.get_host_features	= get_host_features,
>  	.set_guest_features	= set_guest_features,
>  	.init_vq		= init_vq,
> -- 
> 2.25.1
> 

Re: [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL

[Alexandru Elisei]

Hi Martin,

On Tue, Jan 18, 2022 at 12:12:00AM +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 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);
>  	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);
>  
>  	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:
> +		{

The braces aren't necessary 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) {

"The driver notifies the device about new buffers being available in a queue by
writing the index of the updated queue to QueueNotify".

Shouldn't the inequality be val >= vq_count, as the maximum queue index is
vq_count - 1? This is also how virtio-pci does the check for
VIRTIO_PCI_QUEUE_NOTIFY.

> +			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;
>  
> -	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++)

Why this change?

Thanks,
Alex

>  		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;
>  
> -	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;
>  }
> -- 
> 2.25.1
> 

Re: [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled

[Alexandru Elisei]

Hi Martin,

On Tue, Jan 18, 2022 at 12:12:01AM +0200, Martin Radev wrote:
> The virtio_net device is implicitly enabled if user doesn't
> explicitly specify that virtio_net is disabled. This is
> counter-intuitive to how the rest of the virtio commandline
> works.
> 
> For backwards-compatibility, the commandline parameters are
> not changed. Instead, this patch prints out a warning if
> virtio_net is implicitly enabled.
> 
> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
> ---
>  virtio/net.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/virtio/net.c b/virtio/net.c
> index 9a25bfa..ab75d40 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -1002,6 +1002,9 @@ int virtio_net__init(struct kvm *kvm)
>  
>  	if (kvm->cfg.num_net_devices == 0 && kvm->cfg.no_net == 0) {
>  		static struct virtio_net_params net_params;
> +		pr_warning(
> +			"No net devices configured, but no_net not specified. "
> +			"Enabling virtio_net with default network settings...\n");

I don't think this deserves a warning, as there's nothing inherently wrong
with having a default network device. In fact, this is quite helpful for
users who  want to get a VM up and running without complicated setup. I do
agree that at the moment there's nothing to let the user know that a virtio
network device is being created, not in the usage synopsis for the lkvm run
command, nor in the man page for kvmtool, but I don't think that printing a
warning is the solution here.

What I suggest is that instead you print the virtual network interface
parameters that can be useful for a user to know (like IP address and
netmask, or whatever you think might be useful) using pr_debug() after
virtio_net__init_one() is called successfully below.

Thanks,
Alex

>  
>  		net_params = (struct virtio_net_params) {
>  			.guest_ip	= kvm->cfg.guest_ip,
> -- 
> 2.25.1
> 

Re: [PATCH kvmtool 4/5] Makefile: Mark stack as not executable

[Alexandru Elisei]

Hi Martin,

On Tue, Jan 18, 2022 at 12:12:02AM +0200, Martin Radev wrote:
> This patch modifies CFLAGS to mark the stack explicitly
> as not executable.
> 
> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
> ---
>  Makefile | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index f251147..09ef282 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -380,8 +380,11 @@ DEFINES	+= -D_GNU_SOURCE
>  DEFINES	+= -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"'
>  DEFINES	+= -DBUILD_ARCH='"$(ARCH)"'
>  
> +# The stack doesn't need to be executable
> +SECURITY_HARDENINGS := -z noexecstack
> +
>  KVM_INCLUDE := include
> -CFLAGS	+= $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
> +CFLAGS	+= $(CPPFLAGS) $(DEFINES) $(SECURITY_HARDENINGS) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g

I used scanelf to check that the final binary has the stack marked as
executable. For arm and arm64 I got this:

$ scanelf -e lkvm
 TYPE   STK/REL/PTL FILE
ET_DYN RW- R-- RW- lkvm

which as far as I can tell means the stack is not executable.

For x86:

$ scanelf -e lkvm
 TYPE   STK/REL/PTL FILE
ET_DYN RWX R-- RW- vm

which means the stack is executable. Digging further, it looks like there
are two objects which are missing the .note.GNU-stack section,
x86/bios/entry.o and x86/bios/bios-rom.o. I suggest you try to fix the
source files for those two objects before adding the flag to gcc. I used
the Gentoo wiki [1] to diagnose the problem, in case it's useful to you.

[1] https://wiki.gentoo.org/wiki/Hardened/GNU_stack_quickstart

Thanks,
Alex

>  
>  WARNINGS += -Wall
>  WARNINGS += -Wformat=2
> @@ -582,4 +585,4 @@ ifneq ($(MAKECMDGOALS),clean)
>  
>  KVMTOOLS-VERSION-FILE:
>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> -endif
> \ No newline at end of file
> +endif
> -- 
> 2.25.1
> 

Re: [PATCH kvmtool 5/5] mmio: Sanitize addr and len

[Alexandru Elisei]

Hi Martin,

On Tue, Jan 18, 2022 at 12:12:03AM +0200, Martin Radev wrote:
> This patch verifies that adding the addr and length arguments
> from an MMIO op do not overflow. This is necessary because the
> arguments are controlled by the VM. The length may be set to
> an arbitrary value by using the rep prefix.

The Arm architecture doesn'tt have instructions to access 0 bytes of
memory. By "rep prefix" you mean the x86 rep* instructions, right?

> 
> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
> ---
>  mmio.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mmio.c b/mmio.c
> index a6dd3aa..04d2af6 100644
> --- a/mmio.c
> +++ b/mmio.c
> @@ -32,6 +32,10 @@ static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len)
>  {
>  	struct rb_int_node *node;
>  
> +	/* If len is zero or if there's an overflow, the MMIO op is invalid. */
> +	if (len + addr <= addr)

Would you mind rewriting the left side of the inequality to addr + len to match
the argument to rb_int_search_range() below?

Otherwise looks good:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

> +		return NULL;
> +
>  	node = rb_int_search_range(root, addr, addr + len);
>  	if (node == NULL)
>  		return NULL;
> -- 
> 2.25.1
> 

Re: [PATCH kvmtool 5/5] mmio: Sanitize addr and len

[Andre Przywara]

On Tue, 18 Jan 2022 00:12:03 +0200
Martin Radev <martin.b.radev@gmail.com> wrote:

> This patch verifies that adding the addr and length arguments
> from an MMIO op do not overflow. This is necessary because the
> arguments are controlled by the VM. The length may be set to
> an arbitrary value by using the rep prefix.

Mmh, interesting, so does the kernel collate this into one
"giant" KVM_EXIT_MMIO with an arbitrary length? I wonder if there are
assumptions in the MMIO code of len never being bigger than say 16. On
ARM/ARM64 we probably never see len being bigger than 8 on those exits.

But the check is certainly fine anyway...

> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks,
Andre

> ---
>  mmio.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mmio.c b/mmio.c
> index a6dd3aa..04d2af6 100644
> --- a/mmio.c
> +++ b/mmio.c
> @@ -32,6 +32,10 @@ static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len)
>  {
>  	struct rb_int_node *node;
>  
> +	/* If len is zero or if there's an overflow, the MMIO op is invalid. */
> +	if (len + addr <= addr)
> +		return NULL;
> +
>  	node = rb_int_search_range(root, addr, addr + len);
>  	if (node == NULL)
>  		return NULL;

[PATCH kvmtool 4/5] Makefile: Mark stack as not executable

[Martin Radev]

This patch modifies CFLAGS to mark the stack explicitly
as not executable.

Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
 Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index f251147..09ef282 100644
--- a/Makefile
+++ b/Makefile
@@ -380,8 +380,11 @@ DEFINES	+= -D_GNU_SOURCE
 DEFINES	+= -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"'
 DEFINES	+= -DBUILD_ARCH='"$(ARCH)"'
 
+# The stack doesn't need to be executable
+SECURITY_HARDENINGS := -z noexecstack
+
 KVM_INCLUDE := include
-CFLAGS	+= $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
+CFLAGS	+= $(CPPFLAGS) $(DEFINES) $(SECURITY_HARDENINGS) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g
 
 WARNINGS += -Wall
 WARNINGS += -Wformat=2
@@ -582,4 +585,4 @@ ifneq ($(MAKECMDGOALS),clean)
 
 KVMTOOLS-VERSION-FILE:
 	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
-endif
\ No newline at end of file
+endif
-- 
2.25.1