* [PATCH v3 kvmtool 1/6] kvmtool: Add WARN_ONCE macro
2022-05-09 20:39 [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code Martin Radev
@ 2022-05-09 20:39 ` Martin Radev
2022-05-09 20:39 ` [PATCH v3 kvmtool 2/6] mmio: Sanitize addr and len Martin Radev
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Martin Radev @ 2022-05-09 20:39 UTC (permalink / raw)
To: kvm; +Cc: will, alexandru.elisei, Martin Radev
Add a macro to enable to print a warning only once. This is
beneficial for cases where a warning could be helpful for
debugging, but still log pollution is preferred not to happen.
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
include/kvm/util.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/kvm/util.h b/include/kvm/util.h
index d76568a..b494548 100644
--- a/include/kvm/util.h
+++ b/include/kvm/util.h
@@ -82,6 +82,16 @@ do { \
__ret_warn_on; \
})
+#define WARN_ONCE(condition, format, args...) ({ \
+ static int __warned; \
+ int __ret_warn_on = !!(condition); \
+ if (!__warned && __ret_warn_on) { \
+ __warned = 1; \
+ pr_warning(format, args); \
+ } \
+ __ret_warn_on; \
+})
+
#define MSECS_TO_USECS(s) ((s) * 1000)
/* Millisecond sleep */
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 kvmtool 2/6] mmio: Sanitize addr and len
2022-05-09 20:39 [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code Martin Radev
2022-05-09 20:39 ` [PATCH v3 kvmtool 1/6] kvmtool: Add WARN_ONCE macro Martin Radev
@ 2022-05-09 20:39 ` Martin Radev
2022-05-11 17:26 ` Alexandru Elisei
2022-05-09 20:39 ` [PATCH v3 kvmtool 3/6] virtio: Use u32 instead of int in pci_data_in/out Martin Radev
` (4 subsequent siblings)
6 siblings, 1 reply; 10+ messages in thread
From: Martin Radev @ 2022-05-09 20:39 UTC (permalink / raw)
To: kvm; +Cc: will, alexandru.elisei, 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.
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
mmio.c | 4 ++++
virtio/mmio.c | 6 ++++++
2 files changed, 10 insertions(+)
diff --git a/mmio.c b/mmio.c
index a6dd3aa..5a114e9 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 (addr + len <= addr)
+ return NULL;
+
node = rb_int_search_range(root, addr, addr + len);
if (node == NULL)
return NULL;
diff --git a/virtio/mmio.c b/virtio/mmio.c
index 875a288..979fa8c 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -105,6 +105,12 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu,
struct virtio_mmio *vmmio = vdev->virtio;
u32 i;
+ /* Check for wrap-around and zero length. */
+ if (addr + len <= addr) {
+ WARN_ONCE(1, "addr (%llu) + length (%u) wraps-around.\n", addr, len);
+ return;
+ }
+
for (i = 0; i < len; i++) {
if (is_write)
vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] =
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 kvmtool 2/6] mmio: Sanitize addr and len
2022-05-09 20:39 ` [PATCH v3 kvmtool 2/6] mmio: Sanitize addr and len Martin Radev
@ 2022-05-11 17:26 ` Alexandru Elisei
0 siblings, 0 replies; 10+ messages in thread
From: Alexandru Elisei @ 2022-05-11 17:26 UTC (permalink / raw)
To: Martin Radev; +Cc: kvm, will
Hi Martin,
On Mon, May 09, 2022 at 11:39:36PM +0300, 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.
>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
> ---
> mmio.c | 4 ++++
> virtio/mmio.c | 6 ++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/mmio.c b/mmio.c
> index a6dd3aa..5a114e9 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 (addr + len <= addr)
> + return NULL;
> +
> node = rb_int_search_range(root, addr, addr + len);
> if (node == NULL)
> return NULL;
> diff --git a/virtio/mmio.c b/virtio/mmio.c
> index 875a288..979fa8c 100644
> --- a/virtio/mmio.c
> +++ b/virtio/mmio.c
> @@ -105,6 +105,12 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu,
> struct virtio_mmio *vmmio = vdev->virtio;
> u32 i;
>
> + /* Check for wrap-around and zero length. */
> + if (addr + len <= addr) {
> + WARN_ONCE(1, "addr (%llu) + length (%u) wraps-around.\n", addr, len);
> + return;
> + }
This is _NOT_ needed.
When a VCPU exits with exit_reason set to KVM_EXIT_MMIO, kvmtool searches
for the virtio-mmio callback (which ends up calling
virtio_mmio_device_specific) in kvm_cpu__emulate_mmio() ->
kvm__emulate_mmio() -> mmio_search(), which already contains the overflow
check. The virtio_mmio_device_specific() checks above is redundant.
Please remove the above 5 lines of code.
Thanks,
Alex
> +
> for (i = 0; i < len; i++) {
> if (is_write)
> vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] =
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 kvmtool 3/6] virtio: Use u32 instead of int in pci_data_in/out
2022-05-09 20:39 [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code Martin Radev
2022-05-09 20:39 ` [PATCH v3 kvmtool 1/6] kvmtool: Add WARN_ONCE macro Martin Radev
2022-05-09 20:39 ` [PATCH v3 kvmtool 2/6] mmio: Sanitize addr and len Martin Radev
@ 2022-05-09 20:39 ` Martin Radev
2022-05-09 20:39 ` [PATCH v3 kvmtool 4/6] virtio: Sanitize config accesses Martin Radev
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Martin Radev @ 2022-05-09 20:39 UTC (permalink / raw)
To: kvm; +Cc: will, alexandru.elisei, Martin Radev
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.
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 kvmtool 4/6] virtio: Sanitize config accesses
2022-05-09 20:39 [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code Martin Radev
` (2 preceding siblings ...)
2022-05-09 20:39 ` [PATCH v3 kvmtool 3/6] virtio: Use u32 instead of int in pci_data_in/out Martin Radev
@ 2022-05-09 20:39 ` Martin Radev
2022-05-12 10:29 ` Alexandru Elisei
2022-05-09 20:39 ` [PATCH v3 kvmtool 5/6] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev
` (2 subsequent siblings)
6 siblings, 1 reply; 10+ messages in thread
From: Martin Radev @ 2022-05-09 20:39 UTC (permalink / raw)
To: kvm; +Cc: will, alexandru.elisei, 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.
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.
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
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 979fa8c..5ff2a5b 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 and zero length. */
@@ -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..050cfea 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 (%u) 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 (%u) 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 kvmtool 4/6] virtio: Sanitize config accesses
2022-05-09 20:39 ` [PATCH v3 kvmtool 4/6] virtio: Sanitize config accesses Martin Radev
@ 2022-05-12 10:29 ` Alexandru Elisei
0 siblings, 0 replies; 10+ messages in thread
From: Alexandru Elisei @ 2022-05-12 10:29 UTC (permalink / raw)
To: Martin Radev; +Cc: kvm, will
Hi Martin,
On Mon, May 09, 2022 at 11:39:38PM +0300, 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.
>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 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);
This still needs to be a separate patch, as I explained earlier [1]. Something
like this (you don't need to credit me):
---------------------------------------------------------------------(snip)
virtio/9p: Fix virtio_9p_config allocation size
Per the Linux user API, the struct virtio_9p_config "tag" field contains
the non-NULL terminated tag name and this is how the tag name is
copied by kvmtool in virtio_9p__register(). However, the memory allocation
for the struct is off by one, as it allocates memory for the tag name and
the NULL byte. Fix it by reducing the allocation by exactly one byte.
This is also matches how the struct is allocated by QEMU tagged v7.0.0
in virtio_9p_get_config().
diff --git a/virtio/9p.c b/virtio/9p.c
index b78f2b3f0e09..ca83436ae488 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -1577,7 +1577,7 @@ 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);
+ p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name));
if (p9dev->config == NULL) {
err = -ENOMEM;
goto free_p9dev;
---------------------------------------------------------------------(snip)
[1] https://lore.kernel.org/all/YmJ%2FebYEP7tcrxem@monolith.localdoman/
Thanks,
Alex
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 kvmtool 5/6] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL
2022-05-09 20:39 [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code Martin Radev
` (3 preceding siblings ...)
2022-05-09 20:39 ` [PATCH v3 kvmtool 4/6] virtio: Sanitize config accesses Martin Radev
@ 2022-05-09 20:39 ` Martin Radev
2022-05-09 20:39 ` [PATCH v3 kvmtool 6/6] kvmtool: Have stack be not executable on x86 Martin Radev
2022-05-20 20:51 ` [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code Will Deacon
6 siblings, 0 replies; 10+ messages in thread
From: Martin Radev @ 2022-05-09 20:39 UTC (permalink / raw)
To: kvm; +Cc: will, alexandru.elisei, 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 uint since negative
doesn't carry any semantic meaning.
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
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 | 16 +++++++++++++++-
virtio/net.c | 2 +-
virtio/pci.c | 17 +++++++++++++++--
virtio/rng.c | 2 +-
virtio/scsi.c | 2 +-
virtio/vsock.c | 2 +-
11 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
index 3880e74..ad274ac 100644
--- a/include/kvm/virtio.h
+++ b/include/kvm/virtio.h
@@ -187,7 +187,7 @@ struct virtio_ops {
size_t (*get_config_size)(struct kvm *kvm, void *dev);
u32 (*get_host_features)(struct kvm *kvm, void *dev);
void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features);
- int (*get_vq_count)(struct kvm *kvm, void *dev);
+ unsigned int (*get_vq_count)(struct kvm *kvm, void *dev);
int (*init_vq)(struct kvm *kvm, void *dev, u32 vq, u32 page_size,
u32 align, u32 pfn);
void (*exit_vq)(struct kvm *kvm, void *dev, u32 vq);
diff --git a/virtio/9p.c b/virtio/9p.c
index 57cd6d0..7c9d792 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -1469,7 +1469,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
return size;
}
-static int get_vq_count(struct kvm *kvm, void *dev)
+static unsigned int get_vq_count(struct kvm *kvm, void *dev)
{
return NUM_VIRT_QUEUES;
}
diff --git a/virtio/balloon.c b/virtio/balloon.c
index 5bcd6ab..450b36a 100644
--- a/virtio/balloon.c
+++ b/virtio/balloon.c
@@ -251,7 +251,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
return size;
}
-static int get_vq_count(struct kvm *kvm, void *dev)
+static unsigned int get_vq_count(struct kvm *kvm, void *dev)
{
return NUM_VIRT_QUEUES;
}
diff --git a/virtio/blk.c b/virtio/blk.c
index af71c0c..46ee028 100644
--- a/virtio/blk.c
+++ b/virtio/blk.c
@@ -291,7 +291,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
return size;
}
-static int get_vq_count(struct kvm *kvm, void *dev)
+static unsigned int get_vq_count(struct kvm *kvm, void *dev)
{
return NUM_VIRT_QUEUES;
}
diff --git a/virtio/console.c b/virtio/console.c
index dae6034..8315808 100644
--- a/virtio/console.c
+++ b/virtio/console.c
@@ -216,7 +216,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
return size;
}
-static int get_vq_count(struct kvm *kvm, void *dev)
+static unsigned int get_vq_count(struct kvm *kvm, void *dev)
{
return VIRTIO_CONSOLE_NUM_QUEUES;
}
diff --git a/virtio/mmio.c b/virtio/mmio.c
index 5ff2a5b..f2d8630 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -175,13 +175,22 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu,
{
struct virtio_mmio *vmmio = vdev->virtio;
struct kvm *kvm = vmmio->kvm;
+ unsigned int vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev);
u32 val = 0;
switch (addr) {
case VIRTIO_MMIO_HOST_FEATURES_SEL:
case VIRTIO_MMIO_GUEST_FEATURES_SEL:
+ val = ioport__read32(data);
+ *(u32 *)(((void *)&vmmio->hdr) + addr) = val;
+ break;
case VIRTIO_MMIO_QUEUE_SEL:
val = ioport__read32(data);
+ if (val >= vq_count) {
+ WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n",
+ val, vq_count);
+ break;
+ }
*(u32 *)(((void *)&vmmio->hdr) + addr) = val;
break;
case VIRTIO_MMIO_STATUS:
@@ -227,6 +236,11 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu,
break;
case VIRTIO_MMIO_QUEUE_NOTIFY:
val = ioport__read32(data);
+ if (val >= vq_count) {
+ WARN_ONCE(1, "QUEUE_NOTIFY value (%u) is larger than VQ count (%u)\n",
+ val, vq_count);
+ break;
+ }
vdev->ops->notify_vq(vmmio->kvm, vmmio->dev, val);
break;
case VIRTIO_MMIO_INTERRUPT_ACK:
@@ -346,7 +360,7 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev)
{
- int vq;
+ unsigned int vq;
struct virtio_mmio *vmmio = vdev->virtio;
for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++)
diff --git a/virtio/net.c b/virtio/net.c
index ec5dc1f..67070d6 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -755,7 +755,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
return size;
}
-static int get_vq_count(struct kvm *kvm, void *dev)
+static unsigned int get_vq_count(struct kvm *kvm, void *dev)
{
struct net_dev *ndev = dev;
diff --git a/virtio/pci.c b/virtio/pci.c
index 050cfea..23831d5 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -320,9 +320,11 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde
struct virtio_pci *vpci;
struct kvm *kvm;
u32 val;
+ unsigned int vq_count;
kvm = vcpu->kvm;
vpci = vdev->virtio;
+ vq_count = vdev->ops->get_vq_count(kvm, vpci->dev);
switch (offset) {
case VIRTIO_PCI_GUEST_FEATURES:
@@ -342,10 +344,21 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde
}
break;
case VIRTIO_PCI_QUEUE_SEL:
- vpci->queue_selector = ioport__read16(data);
+ val = ioport__read16(data);
+ if (val >= vq_count) {
+ WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n",
+ val, vq_count);
+ return false;
+ }
+ vpci->queue_selector = val;
break;
case VIRTIO_PCI_QUEUE_NOTIFY:
val = ioport__read16(data);
+ if (val >= vq_count) {
+ WARN_ONCE(1, "QUEUE_SEL value (%u) is larger than VQ count (%u)\n",
+ val, vq_count);
+ return false;
+ }
vdev->ops->notify_vq(kvm, vpci->dev, val);
break;
case VIRTIO_PCI_STATUS:
@@ -638,7 +651,7 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev)
{
- int vq;
+ unsigned int vq;
struct virtio_pci *vpci = vdev->virtio;
for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++)
diff --git a/virtio/rng.c b/virtio/rng.c
index c7835a0..75b682e 100644
--- a/virtio/rng.c
+++ b/virtio/rng.c
@@ -147,7 +147,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
return size;
}
-static int get_vq_count(struct kvm *kvm, void *dev)
+static unsigned int get_vq_count(struct kvm *kvm, void *dev)
{
return NUM_VIRT_QUEUES;
}
diff --git a/virtio/scsi.c b/virtio/scsi.c
index 8f1c348..60432cc 100644
--- a/virtio/scsi.c
+++ b/virtio/scsi.c
@@ -176,7 +176,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
return size;
}
-static int get_vq_count(struct kvm *kvm, void *dev)
+static unsigned int get_vq_count(struct kvm *kvm, void *dev)
{
return NUM_VIRT_QUEUES;
}
diff --git a/virtio/vsock.c b/virtio/vsock.c
index 34397b6..64b4e95 100644
--- a/virtio/vsock.c
+++ b/virtio/vsock.c
@@ -204,7 +204,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
die_perror("VHOST_SET_VRING_CALL failed");
}
-static int get_vq_count(struct kvm *kvm, void *dev)
+static unsigned int get_vq_count(struct kvm *kvm, void *dev)
{
return VSOCK_VQ_MAX;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 kvmtool 6/6] kvmtool: Have stack be not executable on x86
2022-05-09 20:39 [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code Martin Radev
` (4 preceding siblings ...)
2022-05-09 20:39 ` [PATCH v3 kvmtool 5/6] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev
@ 2022-05-09 20:39 ` Martin Radev
2022-05-20 20:51 ` [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code Will Deacon
6 siblings, 0 replies; 10+ messages in thread
From: Martin Radev @ 2022-05-09 20:39 UTC (permalink / raw)
To: kvm; +Cc: will, alexandru.elisei, Martin Radev
This patch fixes an issue of having the stack be executable
for x86 builds by ensuring that the two objects bios-rom.o
and entry.o have the section .note.GNU-stack.
Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
x86/bios/bios-rom.S | 5 +++++
x86/bios/entry.S | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/x86/bios/bios-rom.S b/x86/bios/bios-rom.S
index 3269ce9..d1c8b25 100644
--- a/x86/bios/bios-rom.S
+++ b/x86/bios/bios-rom.S
@@ -10,3 +10,8 @@
GLOBAL(bios_rom)
.incbin "x86/bios/bios.bin"
END(bios_rom)
+
+/*
+ * Add this section to ensure final binary has a non-executable stack.
+ */
+.section .note.GNU-stack,"",@progbits
diff --git a/x86/bios/entry.S b/x86/bios/entry.S
index 85056e9..1b71f89 100644
--- a/x86/bios/entry.S
+++ b/x86/bios/entry.S
@@ -90,3 +90,8 @@ GLOBAL(__locals)
#include "local.S"
END(__locals)
+
+/*
+ * Add this section to ensure final binary has a non-executable stack.
+ */
+.section .note.GNU-stack,"",@progbits
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code
2022-05-09 20:39 [PATCH v3 kvmtool 0/6] Fix few small issues in virtio code Martin Radev
` (5 preceding siblings ...)
2022-05-09 20:39 ` [PATCH v3 kvmtool 6/6] kvmtool: Have stack be not executable on x86 Martin Radev
@ 2022-05-20 20:51 ` Will Deacon
6 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2022-05-20 20:51 UTC (permalink / raw)
To: Martin Radev, kvm
Cc: catalin.marinas, kernel-team, Will Deacon, alexandru.elisei
On Mon, 9 May 2022 23:39:34 +0300, Martin Radev wrote:
> Thank you for the patience and for the reviews.
>
> Here is the patchset with all of the changes.
>
> Kind regards,
> Martin
>
> [...]
Applied to kvmtool (master), thanks!
[1/7] kvmtool: Add WARN_ONCE macro
https://git.kernel.org/will/kvmtool/c/143ffa2221d3
[2/7] mmio: Sanitize addr and len
https://git.kernel.org/will/kvmtool/c/52d4ee7cb520
[3/7] virtio: Use u32 instead of int in pci_data_in/out
https://git.kernel.org/will/kvmtool/c/06e1e6fe2e11
[4/7] virtio/9p: Fix virtio_9p_config allocation size
https://git.kernel.org/will/kvmtool/c/3510a7f7b45f
[5/7] virtio: Sanitize config accesses
https://git.kernel.org/will/kvmtool/c/e47302846cc5
[6/7] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL
https://git.kernel.org/will/kvmtool/c/31e0eacca520
[7/7] kvmtool: Have stack be not executable on x86
https://git.kernel.org/will/kvmtool/c/a68a52cd8ab7
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
^ permalink raw reply [flat|nested] 10+ messages in thread