From: Martin Radev <martin.b.radev@gmail.com>
To: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: kvm@vger.kernel.org, will@kernel.org,
julien.thierry.kdev@gmail.com, andre.przywara@arm.com
Subject: Re: [PATCH kvmtool 2/5] virtio: Sanitize config accesses
Date: Sun, 27 Mar 2022 23:37:00 +0300 [thread overview]
Message-ID: <YkDK7L4vU/DpGmCN@sisu-ThinkPad-E14-Gen-2> (raw)
In-Reply-To: <YjHgCrYF20UhtwWc@monolith.localdoman>
Thank you for the review.
Answers are inline.
Here are the two patches:
int to u32 patch:
From ddedd3a59b41d97e07deac59af177b360cc04b20 Mon Sep 17 00:00:00 2001
From: Martin Radev <martin.b.radev@gmail.com>
Date: Thu, 24 Mar 2022 23:24:57 +0200
Subject: [PATCH kvmtool 3/6] virtio: Use u32 instead of int in pci_data_in/out
The PCI access size type is changed from a signed type
to an unsigned type since the size is never expected to
be negative, and the type also matches the type in the
signature of virtio_pci__io_mmio_callback.
This change simplifies size checking in the next patch.
Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
virtio/pci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/virtio/pci.c b/virtio/pci.c
index 2777d1c..bcb205a 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -116,7 +116,7 @@ static inline bool virtio_pci__msix_enabled(struct virtio_pci *vpci)
}
static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device *vdev,
- void *data, int size, unsigned long offset)
+ void *data, u32 size, unsigned long offset)
{
u32 config_offset;
struct virtio_pci *vpci = vdev->virtio;
@@ -146,7 +146,7 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device *
}
static bool virtio_pci__data_in(struct kvm_cpu *vcpu, struct virtio_device *vdev,
- unsigned long offset, void *data, int size)
+ unsigned long offset, void *data, u32 size)
{
bool ret = true;
struct virtio_pci *vpci;
@@ -211,7 +211,7 @@ static void update_msix_map(struct virtio_pci *vpci,
}
static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device *vdev,
- void *data, int size, unsigned long offset)
+ void *data, u32 size, unsigned long offset)
{
struct virtio_pci *vpci = vdev->virtio;
u32 config_offset, vec;
@@ -285,7 +285,7 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device
}
static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vdev,
- unsigned long offset, void *data, int size)
+ unsigned long offset, void *data, u32 size)
{
bool ret = true;
struct virtio_pci *vpci;
--
2.25.1
Original patch but with comments addressed:
From 3a1a16895d7270be1dcb5d8c15607c25e6da670a Mon Sep 17 00:00:00 2001
From: Martin Radev <martin.b.radev@gmail.com>
Date: Sun, 16 Jan 2022 18:19:17 +0200
Subject: [PATCH kvmtool 4/6] virtio: Sanitize config accesses
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@gmail.com>
---
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@gmail.com>
> > ---
> > 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
> >
next prev parent reply other threads:[~2022-03-27 20:37 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-03 23:10 [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Martin Radev
2022-03-03 23:10 ` [PATCH kvmtool 1/5] kvmtool: Add WARN_ONCE macro Martin Radev
2022-03-03 23:10 ` [PATCH kvmtool 2/5] virtio: Sanitize config accesses Martin Radev
2022-03-16 13:04 ` Alexandru Elisei
2022-03-27 20:37 ` Martin Radev [this message]
2022-04-22 10:12 ` Alexandru Elisei
2022-03-03 23:10 ` [PATCH kvmtool 3/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev
2022-03-16 15:38 ` Alexandru Elisei
2022-03-27 20:45 ` Martin Radev
2022-04-22 10:35 ` Alexandru Elisei
2022-03-03 23:10 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev
2022-03-03 23:10 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev
2022-03-16 15:39 ` Alexandru Elisei
2022-03-27 21:00 ` Martin Radev
2022-04-22 10:36 ` Alexandru Elisei
2022-03-10 14:56 ` [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Alexandru Elisei
2022-03-11 11:23 ` Andre Przywara
2022-03-14 17:11 ` Alexandru Elisei
2022-03-27 12:46 ` Martin Radev
2022-04-22 10:37 ` Alexandru Elisei
2022-05-06 13:20 ` Will Deacon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YkDK7L4vU/DpGmCN@sisu-ThinkPad-E14-Gen-2 \
--to=martin.b.radev@gmail.com \
--cc=alexandru.elisei@arm.com \
--cc=andre.przywara@arm.com \
--cc=julien.thierry.kdev@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.