[PATCH kvmtool 4/6] virtio: Sanitize config accesses

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Additionally, PCI accesses which span more than a single byte are prevented
and a warning is printed because the implementation does not currently
support the behavior correctly.

Signed-off-by: Martin Radev <martin.b.radev@xxxxxxxxx>
---
 include/kvm/virtio-9p.h |  1 +
 include/kvm/virtio.h    |  1 +
 virtio/9p.c             | 25 ++++++++++++++++++++-----
 virtio/balloon.c        |  8 ++++++++
 virtio/blk.c            |  8 ++++++++
 virtio/console.c        |  8 ++++++++
 virtio/mmio.c           | 18 ++++++++++++++----
 virtio/net.c            |  8 ++++++++
 virtio/pci.c            | 29 +++++++++++++++++++++++++++++
 virtio/rng.c            |  6 ++++++
 virtio/scsi.c           |  8 ++++++++
 virtio/vsock.c          |  8 ++++++++
 12 files changed, 119 insertions(+), 9 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..57cd6d0 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -1375,6 +1375,13 @@ 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 +1476,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,
@@ -1568,7 +1576,9 @@ virtio_dev_init(virtio_9p__init);
 int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
 {
 	struct p9_dev *p9dev;
-	int err = 0;
+	size_t tag_length;
+	size_t config_size;
+	int err;
 
 	p9dev = calloc(1, sizeof(*p9dev));
 	if (!p9dev)
@@ -1577,29 +1587,34 @@ 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_length = strlen(tag_name);
+	/* The tag_name zero byte is intentionally excluded */
+	config_size = sizeof(*p9dev->config) + tag_length;
+
+	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_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_length);
 
 	list_add(&p9dev->list, &devs);
 
 	if (compat_id == -1)
 		compat_id = virtio_compat_add_message("virtio-9p", "CONFIG_NET_9P_VIRTIO");
 
-	return err;
+	return 0;
 
 free_p9dev_config:
 	free(p9dev->config);
diff --git a/virtio/balloon.c b/virtio/balloon.c
index 8e8803f..5bcd6ab 100644
--- a/virtio/balloon.c
+++ b/virtio/balloon.c
@@ -181,6 +181,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&bdev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	struct bln_dev *bdev = dev;
+
+	return sizeof(bdev->config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	return 1 << VIRTIO_BALLOON_F_STATS_VQ;
@@ -251,6 +258,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..af71c0c 100644
--- a/virtio/blk.c
+++ b/virtio/blk.c
@@ -146,6 +146,13 @@ 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)
+{
+	struct blk_dev *bdev = dev;
+
+	return sizeof(bdev->blk_config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	struct blk_dev *bdev = dev;
@@ -291,6 +298,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..dae6034 100644
--- a/virtio/console.c
+++ b/virtio/console.c
@@ -121,6 +121,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&cdev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	struct con_dev *cdev = dev;
+
+	return sizeof(cdev->config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	return 0;
@@ -216,6 +223,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 5700a89..c14f08a 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -103,6 +103,8 @@ 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;
+	size_t config_size;
 	u32 i;
 
 	/* Check for wrap-around. */
@@ -111,13 +113,21 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu,
 		return;
 	}
 
+	config = vdev->ops->get_config(vmmio->kvm, vmmio->dev);
+	config_size = vdev->ops->get_config_size(vmmio->kvm, vmmio->dev);
+
+	/* Prevent invalid accesses which go beyond the config */
+	if (config_size < addr + len) {
+		WARN_ONCE(1, "Offset (%llu) Length (%u) goes beyond config size (%zu).\n",
+			addr, len, config_size);
+		return;
+	}
+
 	for (i = 0; i < len; i++) {
 		if (is_write)
-			vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] =
-					      *(u8 *)data + i;
+			config[addr + i] = *(u8 *)data + i;
 		else
-			data[i] = vdev->ops->get_config(vmmio->kvm,
-							vmmio->dev)[addr + i];
+			data[i] = config[addr + i];
 	}
 }
 
diff --git a/virtio/net.c b/virtio/net.c
index 1ee3c19..ec5dc1f 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -480,6 +480,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&ndev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	struct net_dev *ndev = dev;
+
+	return sizeof(ndev->config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	u32 features;
@@ -757,6 +764,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 bcb205a..13f2b76 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -136,7 +136,21 @@ 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 + size > config_size) {
+			/* Access goes beyond the config size, so return failure. */
+			WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n",
+				config_offset, config_size);
+			return false;
+		}
 
+		/* TODO: Handle access lengths beyond one byte */
+		if (size != 1) {
+			WARN_ONCE(1, "Size (%d) not supported\n", size);
+			return false;
+		}
 		cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset];
 		ioport__write8(data, cfg);
 		return true;
@@ -276,6 +290,21 @@ 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 + size > config_size) {
+			/* Access goes beyond the config size, so return failure. */
+			WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n",
+				config_offset, config_size);
+			return false;
+		}
+
+		/* TODO: Handle access lengths beyond one byte */
+		if (size != 1) {
+			WARN_ONCE(1, "Size (%d) not supported\n", 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..8f1c348 100644
--- a/virtio/scsi.c
+++ b/virtio/scsi.c
@@ -38,6 +38,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&sdev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	struct scsi_dev *sdev = dev;
+
+	return sizeof(sdev->config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	return	1UL << VIRTIO_RING_F_EVENT_IDX |
@@ -176,6 +183,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..34397b6 100644
--- a/virtio/vsock.c
+++ b/virtio/vsock.c
@@ -41,6 +41,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
 	return ((u8 *)(&vdev->config));
 }
 
+static size_t get_config_size(struct kvm *kvm, void *dev)
+{
+	struct vsock_dev *vdev = dev;
+
+	return sizeof(vdev->config);
+}
+
 static u32 get_host_features(struct kvm *kvm, void *dev)
 {
 	return 1UL << VIRTIO_RING_F_EVENT_IDX
@@ -204,6 +211,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




On Wed, Mar 16, 2022 at 01:04:08PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Fri, Mar 04, 2022 at 01:10:47AM +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.
> > 
> > Additionally, PCI accesses which span more than a single byte are prevented
> > and a warning is printed because the implementation does not currently
> > support the behavior correctly.
> > 
> > Signed-off-by: Martin Radev <martin.b.radev@xxxxxxxxx>
> > ---
> >  include/kvm/virtio-9p.h |  1 +
> >  include/kvm/virtio.h    |  1 +
> >  virtio/9p.c             | 25 ++++++++++++++++++++-----
> >  virtio/balloon.c        |  8 ++++++++
> >  virtio/blk.c            |  8 ++++++++
> >  virtio/console.c        |  8 ++++++++
> >  virtio/mmio.c           | 24 ++++++++++++++++++++----
> >  virtio/net.c            |  8 ++++++++
> >  virtio/pci.c            | 38 ++++++++++++++++++++++++++++++++++++++
> >  virtio/rng.c            |  6 ++++++
> >  virtio/scsi.c           |  8 ++++++++
> >  virtio/vsock.c          |  8 ++++++++
> >  12 files changed, 134 insertions(+), 9 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..6074f3a 100644
> > --- a/virtio/9p.c
> > +++ b/virtio/9p.c
> > @@ -1375,6 +1375,13 @@ 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 +1476,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,
> > @@ -1568,7 +1576,9 @@ virtio_dev_init(virtio_9p__init);
> >  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;
> 
> I think it would be better to name the variable tag_len, the same name as
> the corresponding field in struct virtio_9p_config. As a bonus, it's also
> shorter. But this is personal preference in the end, so I leave it up to
> you to decide which works better.
> 
Done.

> > +	size_t config_size;
> > +	int err;
> >  
> >  	p9dev = calloc(1, sizeof(*p9dev));
> >  	if (!p9dev)
> > @@ -1577,29 +1587,34 @@ 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);
> > +	/* The tag_name zero byte is intentionally excluded */
> 
> If this is indeed a bug (the comment from virtio_9p_config seems to suggest
> it is, but I couldn't find the 9p spec), the bug is that the config size is
> computed incorrectly, which is a different bug than a guest being able to
> write outside of the config region for the device. As such, it should be
> fixed in a separate patch.
> 
I couldn't find information about how large the configuration size is and
whether the 0 byte is included. QEMU explicitly excludes it.
See https://elixir.bootlin.com/qemu/latest/source/hw/9pfs/virtio-9p-device.c#L218
I think this is almost surely the correct way considering the tag length
is also part of the config.

> > +	config_size = sizeof(*p9dev->config) + tag_name_length;
> > +
> > +	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);
> >  
> >  	if (compat_id == -1)
> >  		compat_id = virtio_compat_add_message("virtio-9p", "CONFIG_NET_9P_VIRTIO");
> >  
> > -	return err;
> > +	return 0;
> >  
> >  free_p9dev_config:
> >  	free(p9dev->config);
> > diff --git a/virtio/balloon.c b/virtio/balloon.c
> > index 8e8803f..5bcd6ab 100644
> > --- a/virtio/balloon.c
> > +++ b/virtio/balloon.c
> > @@ -181,6 +181,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> >  	return ((u8 *)(&bdev->config));
> >  }
> >  
> > +static size_t get_config_size(struct kvm *kvm, void *dev)
> > +{
> > +	struct bln_dev *bdev = dev;
> > +
> > +	return sizeof(bdev->config);
> > +}
> > +
> >  static u32 get_host_features(struct kvm *kvm, void *dev)
> >  {
> >  	return 1 << VIRTIO_BALLOON_F_STATS_VQ;
> > @@ -251,6 +258,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..af71c0c 100644
> > --- a/virtio/blk.c
> > +++ b/virtio/blk.c
> > @@ -146,6 +146,13 @@ 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)
> > +{
> > +	struct blk_dev *bdev = dev;
> > +
> > +	return sizeof(bdev->blk_config);
> > +}
> > +
> >  static u32 get_host_features(struct kvm *kvm, void *dev)
> >  {
> >  	struct blk_dev *bdev = dev;
> > @@ -291,6 +298,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..dae6034 100644
> > --- a/virtio/console.c
> > +++ b/virtio/console.c
> > @@ -121,6 +121,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> >  	return ((u8 *)(&cdev->config));
> >  }
> >  
> > +static size_t get_config_size(struct kvm *kvm, void *dev)
> > +{
> > +	struct con_dev *cdev = dev;
> > +
> > +	return sizeof(cdev->config);
> > +}
> > +
> >  static u32 get_host_features(struct kvm *kvm, void *dev)
> >  {
> >  	return 0;
> > @@ -216,6 +223,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..0094856 100644
> > --- a/virtio/mmio.c
> > +++ b/virtio/mmio.c
> > @@ -103,15 +103,31 @@ 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;
> > +	size_t config_aperture_size;
> 
> These could be shortened to config and config_size, to better match the
> callback names (get_config, respectively get_config_size) and make the code
> easier to understand.
> 
Done.

> >  	u32 i;
> >  
> > +	/* Check for wrap-around. */
> > +	if (addr + len < addr) {
> 
> No need for this, you can move patch #5 before this one, and that should
> take care of any wrap arounds.
> 
Done.

> > +		WARN_ONCE(1, "addr (%llu) + length (%u) wraps-around.\n", addr, len);
> > +		return;
> > +	}
> > +
> > +	config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev);
> > +	config_aperture_size = vdev->ops->get_config_size(vmmio->kvm, vmmio->dev);
> > +
> > +	/* Prevent invalid accesses which go beyond the config */
> > +	if (config_aperture_size < addr + len) {
> > +		WARN_ONCE(1, "Offset (%llu) Length (%u) goes beyond config size (%zu).\n",
> > +			addr, len, config_aperture_size);
> > +		return;
> > +	}
> > +
> >  	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..ec5dc1f 100644
> > --- a/virtio/net.c
> > +++ b/virtio/net.c
> > @@ -480,6 +480,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> >  	return ((u8 *)(&ndev->config));
> >  }
> >  
> > +static size_t get_config_size(struct kvm *kvm, void *dev)
> > +{
> > +	struct net_dev *ndev = dev;
> > +
> > +	return sizeof(ndev->config);
> > +}
> > +
> >  static u32 get_host_features(struct kvm *kvm, void *dev)
> >  {
> >  	u32 features;
> > @@ -757,6 +764,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 2777d1c..0b5cccd 100644
> > --- a/virtio/pci.c
> > +++ b/virtio/pci.c
> > @@ -136,7 +136,25 @@ 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 (size <= 0) {
> > +			WARN_ONCE(1, "Size (%d) is non-positive\n", size);
> > +			return false;
> > +		}
> 
> This is a different bug. The kvm_run.mmio struct report length as an u32
> and the type is preserved until virtio_pci__data_{in,out} are called from
> virtio_pci__mmio_callback(). The correct fix is to change the size
> parameter from virtio_pci__data_{in,out} and
> virtio_pci__specific_data_{in,out} to an u32 in a separate patch.

I addressed this as you suggested. Patch is attached.

> 
> [1] https://elixir.bootlin.com/linux/v5.17-rc8/source/Documentation/virt/kvm/api.rst#L5726
> 
> Thanks,
> Alex
> 
> > +		if (config_offset + (u32)size > config_size) {
> > +			/* Access goes beyond the config size, so return failure. */
> > +			WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n",
> > +				config_offset, config_size);
> > +			return false;
> > +		}
> > +
> > +		/* TODO: Handle access lengths beyond one byte */
> > +		if (size != 1) {
> > +			WARN_ONCE(1, "Size (%d) not supported\n", size);
> > +			return false;
> > +		}
> >  		cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset];
> >  		ioport__write8(data, cfg);
> >  		return true;
> > @@ -276,6 +294,26 @@ 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;
> > +
> > +		if (size <= 0) {
> > +			WARN_ONCE(1, "Size (%d) is non-positive\n", size);
> > +			return false;
> > +		}
> > +
> > +		config_size = vdev->ops->get_config_size(kvm, vpci->dev);
> > +		if (config_offset + (u32)size > config_size) {
> > +			/* Access goes beyond the config size, so return failure. */
> > +			WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n",
> > +				config_offset, config_size);
> > +			return false;
> > +		}
> > +
> > +		/* TODO: Handle access lengths beyond one byte */
> > +		if (size != 1) {
> > +			WARN_ONCE(1, "Size (%d) not supported\n", 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..8f1c348 100644
> > --- a/virtio/scsi.c
> > +++ b/virtio/scsi.c
> > @@ -38,6 +38,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> >  	return ((u8 *)(&sdev->config));
> >  }
> >  
> > +static size_t get_config_size(struct kvm *kvm, void *dev)
> > +{
> > +	struct scsi_dev *sdev = dev;
> > +
> > +	return sizeof(sdev->config);
> > +}
> > +
> >  static u32 get_host_features(struct kvm *kvm, void *dev)
> >  {
> >  	return	1UL << VIRTIO_RING_F_EVENT_IDX |
> > @@ -176,6 +183,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..34397b6 100644
> > --- a/virtio/vsock.c
> > +++ b/virtio/vsock.c
> > @@ -41,6 +41,13 @@ static u8 *get_config(struct kvm *kvm, void *dev)
> >  	return ((u8 *)(&vdev->config));
> >  }
> >  
> > +static size_t get_config_size(struct kvm *kvm, void *dev)
> > +{
> > +	struct vsock_dev *vdev = dev;
> > +
> > +	return sizeof(vdev->config);
> > +}
> > +
> >  static u32 get_host_features(struct kvm *kvm, void *dev)
> >  {
> >  	return 1UL << VIRTIO_RING_F_EVENT_IDX
> > @@ -204,6 +211,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
> > 



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux