* [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code
@ 2022-03-03 23:10 Martin Radev
2022-03-03 23:10 ` [PATCH kvmtool 1/5] kvmtool: Add WARN_ONCE macro Martin Radev
` (6 more replies)
0 siblings, 7 replies; 24+ messages in thread
From: Martin Radev @ 2022-03-03 23:10 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev, andre.przywara, alexandru.elisei
Cc: Martin Radev
Hello everyone,
Thanks for the reviews in the first patch set.
This is the second version of the original patch set which addresses
few found overflows in the common virtio code. Since the first version,
the following changes were made:
- the virtio_net warning patch was removed.
- a WARN_ONCE macro is added to help signal that an issue was observed,
but without polluting the log.
- a couple of improvements in sanitization and style.
- TODO comment for missing handling of multi-byte PCI accesses.
The Makefile change is kept in its original form because I didn't understand
if there is an issue with it on aarch64.
Martin Radev (5):
kvmtool: Add WARN_ONCE macro
virtio: Sanitize config accesses
virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL
Makefile: Mark stack as not executable
mmio: Sanitize addr and len
Makefile | 7 +++--
include/kvm/util.h | 10 +++++++
include/kvm/virtio-9p.h | 1 +
include/kvm/virtio.h | 3 ++-
mmio.c | 4 +++
virtio/9p.c | 27 ++++++++++++++-----
virtio/balloon.c | 10 ++++++-
virtio/blk.c | 10 ++++++-
virtio/console.c | 10 ++++++-
virtio/mmio.c | 44 +++++++++++++++++++++++++-----
virtio/net.c | 12 +++++++--
virtio/pci.c | 59 ++++++++++++++++++++++++++++++++++++++---
virtio/rng.c | 8 +++++-
virtio/scsi.c | 10 ++++++-
virtio/vsock.c | 10 ++++++-
15 files changed, 199 insertions(+), 26 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH kvmtool 1/5] kvmtool: Add WARN_ONCE macro 2022-03-03 23:10 [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Martin Radev @ 2022-03-03 23:10 ` Martin Radev 2022-03-03 23:10 ` [PATCH kvmtool 2/5] virtio: Sanitize config accesses Martin Radev ` (5 subsequent siblings) 6 siblings, 0 replies; 24+ messages in thread From: Martin Radev @ 2022-03-03 23:10 UTC (permalink / raw) To: kvm, will, julien.thierry.kdev, andre.przywara, alexandru.elisei Cc: 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. 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] 24+ messages in thread
* [PATCH kvmtool 2/5] virtio: Sanitize config accesses 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 ` Martin Radev 2022-03-16 13:04 ` Alexandru Elisei 2022-03-03 23:10 ` [PATCH kvmtool 3/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev ` (4 subsequent siblings) 6 siblings, 1 reply; 24+ messages in thread From: Martin Radev @ 2022-03-03 23:10 UTC (permalink / raw) To: kvm, will, julien.thierry.kdev, andre.przywara, alexandru.elisei Cc: 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. 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; + 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 */ + 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; u32 i; + /* Check for wrap-around. */ + if (addr + len < addr) { + 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; + } + 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH kvmtool 2/5] virtio: Sanitize config accesses 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 0 siblings, 1 reply; 24+ messages in thread From: Alexandru Elisei @ 2022-03-16 13:04 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev, andre.przywara 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. > + 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. > + 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. > 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. > + 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. [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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH kvmtool 2/5] virtio: Sanitize config accesses 2022-03-16 13:04 ` Alexandru Elisei @ 2022-03-27 20:37 ` Martin Radev 2022-04-22 10:12 ` Alexandru Elisei 0 siblings, 1 reply; 24+ messages in thread From: Martin Radev @ 2022-03-27 20:37 UTC (permalink / raw) To: Alexandru Elisei; +Cc: kvm, will, julien.thierry.kdev, andre.przywara 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 > > ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH kvmtool 2/5] virtio: Sanitize config accesses 2022-03-27 20:37 ` Martin Radev @ 2022-04-22 10:12 ` Alexandru Elisei 0 siblings, 0 replies; 24+ messages in thread From: Alexandru Elisei @ 2022-04-22 10:12 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev, andre.przywara Hi, On Sun, Mar 27, 2022 at 11:37:00PM +0300, Martin Radev wrote: > > 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 That looks good to me. Please send it as a separate patch in the next version of the series. > > Original patch but with comments addressed: If you change the patch, the new version should be sent as a new series. The version of the series should be reflected in the subject of each patch. For example, this series should have been v2, which means the prefix for the patches should have been: PATCH v2 kvmtool [..]. This can be accomplished with git format-patch directly, by using the command line argument --subject-prefix="PATCH v2 kvmtool". The next iteration of the series will be v3, and so on. This is done to make it easier for everyone to keep track of the latest version of a patch set. It's also easier to review and test a new version of a patch when it is standalone than when it is attached to an email containing several other things. For example, you've attached two patches to this email, which can cause confusion about the order of the patches. This can be easily avoided by sending the changes in a separate series. I'll reply to your comments below. > 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. I think I haven't managed to make myself clear. I agree that the NUL terminating byte shouldn't be taken into account when calculating the config size. What I was referring to is the fact that there two different bugs here: 1. The config size is calculated incorrectly in the existing code: p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1); The code shouldn't add that one byte to the size, which presumably represent the NUL terminating byte from the tag_name string. 2. The code doesn't check for overflow. Your patch tries to fix both bugs in one go. What I was suggesting is to write a standalone patch that fixes bug #1, and keep this patch that adds the overflow check, minus the fix for #1. This makes everything cleaner, easier to review and test, and easier to diagnose and fix or revert if the fix turns out to be wrong. Thanks, Alex ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH kvmtool 3/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL 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-03 23:10 ` Martin Radev 2022-03-16 15:38 ` Alexandru Elisei 2022-03-03 23:10 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev ` (3 subsequent siblings) 6 siblings, 1 reply; 24+ messages in thread From: Martin Radev @ 2022-03-03 23:10 UTC (permalink / raw) To: kvm, will, julien.thierry.kdev, andre.przywara, alexandru.elisei Cc: 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. 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 | 20 ++++++++++++++++++-- virtio/net.c | 4 ++-- virtio/pci.c | 21 ++++++++++++++++++--- virtio/rng.c | 2 +- virtio/scsi.c | 2 +- virtio/vsock.c | 2 +- 11 files changed, 46 insertions(+), 15 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 6074f3a..7374f1e 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 0094856..d3555b4 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,10 +360,12 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev, int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev) { - int vq; + unsigned int vq; struct virtio_mmio *vmmio = vdev->virtio; + unsigned int vq_count; - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++) + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); + for (vq = 0; vq < vq_count; vq++) virtio_mmio_exit_vq(kvm, vdev, vq); return 0; diff --git a/virtio/net.c b/virtio/net.c index ec5dc1f..8dd523f 100644 --- a/virtio/net.c +++ b/virtio/net.c @@ -755,11 +755,11 @@ 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; - return ndev->queue_pairs * 2 + 1; + return ndev->queue_pairs * 2U + 1U; } static struct virtio_ops net_dev_virtio_ops = { diff --git a/virtio/pci.c b/virtio/pci.c index 0b5cccd..9a6cbf3 100644 --- a/virtio/pci.c +++ b/virtio/pci.c @@ -329,9 +329,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: @@ -351,10 +353,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: @@ -647,10 +660,12 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev, int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev) { - int vq; + unsigned int vq; + unsigned int vq_count; struct virtio_pci *vpci = vdev->virtio; - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++) + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); + for (vq = 0; vq < vq_count; vq++) virtio_pci_exit_vq(kvm, vdev, vq); return 0; diff --git a/virtio/rng.c b/virtio/rng.c index c7835a0..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] 24+ messages in thread
* Re: [PATCH kvmtool 3/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL 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 0 siblings, 1 reply; 24+ messages in thread From: Alexandru Elisei @ 2022-03-16 15:38 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev, andre.przywara Hi, On Fri, Mar 04, 2022 at 01:10:48AM +0200, Martin Radev wrote: > This patch checks for overflows in QUEUE_NOTIFY and QUEUE_SEL in > the PCI and MMIO operation handling paths. Further, the return > value type of get_vq_count is changed from int to uint since negative > doesn't carry any semantic meaning. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > include/kvm/virtio.h | 2 +- > virtio/9p.c | 2 +- > virtio/balloon.c | 2 +- > virtio/blk.c | 2 +- > virtio/console.c | 2 +- > virtio/mmio.c | 20 ++++++++++++++++++-- > virtio/net.c | 4 ++-- > virtio/pci.c | 21 ++++++++++++++++++--- > virtio/rng.c | 2 +- > virtio/scsi.c | 2 +- > virtio/vsock.c | 2 +- > 11 files changed, 46 insertions(+), 15 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 6074f3a..7374f1e 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 0094856..d3555b4 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,10 +360,12 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev) > { > - int vq; > + unsigned int vq; > struct virtio_mmio *vmmio = vdev->virtio; > + unsigned int vq_count; > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++) > + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); > + for (vq = 0; vq < vq_count; vq++) Nitpick: this change is unnecessary and pollutes the git history for this file. Same for virtio_pci_reset() below. > virtio_mmio_exit_vq(kvm, vdev, vq); > > return 0; > diff --git a/virtio/net.c b/virtio/net.c > index ec5dc1f..8dd523f 100644 > --- a/virtio/net.c > +++ b/virtio/net.c > @@ -755,11 +755,11 @@ 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; > > - return ndev->queue_pairs * 2 + 1; > + return ndev->queue_pairs * 2U + 1U; I don't think the cast is needed, as far as I know signed integers are converted to unsigned integers as far back as C89 (and probably even before that). Other than the nitpicks above, the patch looks good. Thanks, Alex > } > > static struct virtio_ops net_dev_virtio_ops = { > diff --git a/virtio/pci.c b/virtio/pci.c > index 0b5cccd..9a6cbf3 100644 > --- a/virtio/pci.c > +++ b/virtio/pci.c > @@ -329,9 +329,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: > @@ -351,10 +353,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: > @@ -647,10 +660,12 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev) > { > - int vq; > + unsigned int vq; > + unsigned int vq_count; > struct virtio_pci *vpci = vdev->virtio; > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++) > + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); > + for (vq = 0; vq < vq_count; vq++) > virtio_pci_exit_vq(kvm, vdev, vq); > > return 0; > diff --git a/virtio/rng.c b/virtio/rng.c > index c7835a0..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 [flat|nested] 24+ messages in thread
* Re: [PATCH kvmtool 3/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL 2022-03-16 15:38 ` Alexandru Elisei @ 2022-03-27 20:45 ` Martin Radev 2022-04-22 10:35 ` Alexandru Elisei 0 siblings, 1 reply; 24+ messages in thread From: Martin Radev @ 2022-03-27 20:45 UTC (permalink / raw) To: Alexandru Elisei; +Cc: kvm, will, julien.thierry.kdev, andre.przywara Thanks for the review. Comments are inline. Here is the patch: From dc2b54f3368bd012eaa2f55bb8b98278f6278df1 Mon Sep 17 00:00:00 2001 From: Martin Radev <martin.b.radev@gmail.com> Date: Sun, 16 Jan 2022 18:50:44 +0200 Subject: [PATCH kvmtool 5/6] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL 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. 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 c14f08a..4c16359 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 13f2b76..eb7af32 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 On Wed, Mar 16, 2022 at 03:38:19PM +0000, Alexandru Elisei wrote: > Hi, > > On Fri, Mar 04, 2022 at 01:10:48AM +0200, Martin Radev wrote: > > This patch checks for overflows in QUEUE_NOTIFY and QUEUE_SEL in > > the PCI and MMIO operation handling paths. Further, the return > > value type of get_vq_count is changed from int to uint since negative > > doesn't carry any semantic meaning. > > > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > > --- > > include/kvm/virtio.h | 2 +- > > virtio/9p.c | 2 +- > > virtio/balloon.c | 2 +- > > virtio/blk.c | 2 +- > > virtio/console.c | 2 +- > > virtio/mmio.c | 20 ++++++++++++++++++-- > > virtio/net.c | 4 ++-- > > virtio/pci.c | 21 ++++++++++++++++++--- > > virtio/rng.c | 2 +- > > virtio/scsi.c | 2 +- > > virtio/vsock.c | 2 +- > > 11 files changed, 46 insertions(+), 15 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 6074f3a..7374f1e 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 0094856..d3555b4 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,10 +360,12 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > > > int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev) > > { > > - int vq; > > + unsigned int vq; > > struct virtio_mmio *vmmio = vdev->virtio; > > + unsigned int vq_count; > > > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++) > > + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); > > + for (vq = 0; vq < vq_count; vq++) > > Nitpick: this change is unnecessary and pollutes the git history for this > file. Same for virtio_pci_reset() below. > I thought it would be nicer to not call this function on every loop iterations. Still, I removed it as you suggested. > > virtio_mmio_exit_vq(kvm, vdev, vq); > > > > return 0; > > diff --git a/virtio/net.c b/virtio/net.c > > index ec5dc1f..8dd523f 100644 > > --- a/virtio/net.c > > +++ b/virtio/net.c > > @@ -755,11 +755,11 @@ 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; > > > > - return ndev->queue_pairs * 2 + 1; > > + return ndev->queue_pairs * 2U + 1U; > > I don't think the cast is needed, as far as I know signed integers are > converted to unsigned integers as far back as C89 (and probably even > before that). Done. > > Other than the nitpicks above, the patch looks good. > > Thanks, > Alex > > > } > > > > static struct virtio_ops net_dev_virtio_ops = { > > diff --git a/virtio/pci.c b/virtio/pci.c > > index 0b5cccd..9a6cbf3 100644 > > --- a/virtio/pci.c > > +++ b/virtio/pci.c > > @@ -329,9 +329,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: > > @@ -351,10 +353,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: > > @@ -647,10 +660,12 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > > > int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev) > > { > > - int vq; > > + unsigned int vq; > > + unsigned int vq_count; > > struct virtio_pci *vpci = vdev->virtio; > > > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++) > > + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); > > + for (vq = 0; vq < vq_count; vq++) > > virtio_pci_exit_vq(kvm, vdev, vq); > > > > return 0; > > diff --git a/virtio/rng.c b/virtio/rng.c > > index c7835a0..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] 24+ messages in thread
* Re: [PATCH kvmtool 3/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL 2022-03-27 20:45 ` Martin Radev @ 2022-04-22 10:35 ` Alexandru Elisei 0 siblings, 0 replies; 24+ messages in thread From: Alexandru Elisei @ 2022-04-22 10:35 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev, andre.przywara Hi, On Sun, Mar 27, 2022 at 11:45:13PM +0300, Martin Radev wrote: > > Thanks for the review. > Comments are inline. > Here is the patch: Would you mind making a new version of the series to send this patch? Thanks, Alex ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH kvmtool 4/5] Makefile: Mark stack as not executable 2022-03-03 23:10 [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Martin Radev ` (2 preceding siblings ...) 2022-03-03 23:10 ` [PATCH kvmtool 3/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev @ 2022-03-03 23:10 ` Martin Radev 2022-03-03 23:10 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev ` (2 subsequent siblings) 6 siblings, 0 replies; 24+ messages in thread From: Martin Radev @ 2022-03-03 23:10 UTC (permalink / raw) To: kvm, will, julien.thierry.kdev, andre.przywara, alexandru.elisei Cc: Martin Radev This patch modifies CFLAGS to mark the stack explicitly as not executable. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- Makefile | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index f251147..09ef282 100644 --- a/Makefile +++ b/Makefile @@ -380,8 +380,11 @@ DEFINES += -D_GNU_SOURCE DEFINES += -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"' DEFINES += -DBUILD_ARCH='"$(ARCH)"' +# The stack doesn't need to be executable +SECURITY_HARDENINGS := -z noexecstack + KVM_INCLUDE := include -CFLAGS += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g +CFLAGS += $(CPPFLAGS) $(DEFINES) $(SECURITY_HARDENINGS) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g WARNINGS += -Wall WARNINGS += -Wformat=2 @@ -582,4 +585,4 @@ ifneq ($(MAKECMDGOALS),clean) KVMTOOLS-VERSION-FILE: @$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT) -endif \ No newline at end of file +endif -- 2.25.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH kvmtool 5/5] mmio: Sanitize addr and len 2022-03-03 23:10 [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Martin Radev ` (3 preceding siblings ...) 2022-03-03 23:10 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev @ 2022-03-03 23:10 ` Martin Radev 2022-03-16 15:39 ` Alexandru Elisei 2022-03-10 14:56 ` [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Alexandru Elisei 2022-05-06 13:20 ` Will Deacon 6 siblings, 1 reply; 24+ messages in thread From: Martin Radev @ 2022-03-03 23:10 UTC (permalink / raw) To: kvm, will, julien.thierry.kdev, andre.przywara, alexandru.elisei Cc: Martin Radev This patch verifies that adding the addr and length arguments from an MMIO op do not overflow. This is necessary because the arguments are controlled by the VM. The length may be set to an arbitrary value by using the rep prefix. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- mmio.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/mmio.c b/mmio.c index a6dd3aa..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; -- 2.25.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH kvmtool 5/5] mmio: Sanitize addr and len 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 0 siblings, 1 reply; 24+ messages in thread From: Alexandru Elisei @ 2022-03-16 15:39 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev, andre.przywara Hi, On Fri, Mar 04, 2022 at 01:10:50AM +0200, Martin Radev wrote: > This patch verifies that adding the addr and length arguments > from an MMIO op do not overflow. This is necessary because the > arguments are controlled by the VM. The length may be set to > an arbitrary value by using the rep prefix. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> The patch looks correct to me: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex > --- > mmio.c | 4 ++++ > 1 file changed, 4 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; > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH kvmtool 5/5] mmio: Sanitize addr and len 2022-03-16 15:39 ` Alexandru Elisei @ 2022-03-27 21:00 ` Martin Radev 2022-04-22 10:36 ` Alexandru Elisei 0 siblings, 1 reply; 24+ messages in thread From: Martin Radev @ 2022-03-27 21:00 UTC (permalink / raw) To: Alexandru Elisei; +Cc: kvm, will, julien.thierry.kdev, andre.przywara Thanks Alex. I needed to make a small update to the patch as you recommented in one of the other emails. I still kept the reviewed by line with your name. From 090f373c0bc868cc4551620568d47b21b6ac044a Mon Sep 17 00:00:00 2001 From: Martin Radev <martin.b.radev@gmail.com> Date: Mon, 17 Jan 2022 23:17:25 +0200 Subject: [PATCH kvmtool 2/6] mmio: Sanitize addr and len 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 On Wed, Mar 16, 2022 at 03:39:55PM +0000, Alexandru Elisei wrote: > Hi, > > On Fri, Mar 04, 2022 at 01:10:50AM +0200, Martin Radev wrote: > > This patch verifies that adding the addr and length arguments > > from an MMIO op do not overflow. This is necessary because the > > arguments are controlled by the VM. The length may be set to > > an arbitrary value by using the rep prefix. > > > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > > The patch looks correct to me: > > Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> > > Thanks, > Alex > > > --- > > mmio.c | 4 ++++ > > 1 file changed, 4 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; > > -- > > 2.25.1 > > ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH kvmtool 5/5] mmio: Sanitize addr and len 2022-03-27 21:00 ` Martin Radev @ 2022-04-22 10:36 ` Alexandru Elisei 0 siblings, 0 replies; 24+ messages in thread From: Alexandru Elisei @ 2022-04-22 10:36 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev, andre.przywara Hi, On Mon, Mar 28, 2022 at 12:00:30AM +0300, Martin Radev wrote: > Thanks Alex. > > I needed to make a small update to the patch as you recommented in one > of the other emails. I still kept the reviewed by line with your name. That's quite fine, please send it in a new iteration of the series. Thanks, Alex > > From 090f373c0bc868cc4551620568d47b21b6ac044a Mon Sep 17 00:00:00 2001 > From: Martin Radev <martin.b.radev@gmail.com> > Date: Mon, 17 Jan 2022 23:17:25 +0200 > Subject: [PATCH kvmtool 2/6] mmio: Sanitize addr and len > > 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 > > On Wed, Mar 16, 2022 at 03:39:55PM +0000, Alexandru Elisei wrote: > > Hi, > > > > On Fri, Mar 04, 2022 at 01:10:50AM +0200, Martin Radev wrote: > > > This patch verifies that adding the addr and length arguments > > > from an MMIO op do not overflow. This is necessary because the > > > arguments are controlled by the VM. The length may be set to > > > an arbitrary value by using the rep prefix. > > > > > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > > > > The patch looks correct to me: > > > > Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> > > > > Thanks, > > Alex > > > > > --- > > > mmio.c | 4 ++++ > > > 1 file changed, 4 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; > > > -- > > > 2.25.1 > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code 2022-03-03 23:10 [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Martin Radev ` (4 preceding siblings ...) 2022-03-03 23:10 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev @ 2022-03-10 14:56 ` Alexandru Elisei 2022-03-11 11:23 ` Andre Przywara 2022-05-06 13:20 ` Will Deacon 6 siblings, 1 reply; 24+ messages in thread From: Alexandru Elisei @ 2022-03-10 14:56 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev, andre.przywara Hi Martin, On Fri, Mar 04, 2022 at 01:10:45AM +0200, Martin Radev wrote: > Hello everyone, > [..] > The Makefile change is kept in its original form because I didn't understand > if there is an issue with it on aarch64. I'll try to explain it better. According to this blogpost about executable stacks [1], gcc marks the stack as executable automatically for assembly (.S) files. C files have their stack mark as non-executable by default. If any of the object files have the stack executable, then the resulting binary also has the stack marked as executable (obviously). To mark the stack as non-executable in assembly files, the empty section .note.GNU-stack must be present in the file. This is a marking to tell the linker that the final executable does not require an executable stack. When the linker finds this section, it will create a PT_GNU_STACK empty segment in the final executable. This segment tells Linux to mark the stack as non-executable when it loads the binary. The only assembly files that kvmtool compiles into objects are the x86 files x86/bios/entry.S and x86/bios/bios-rom.S; the other architectures are not affected by this. I haven't found any instances where these files (and the other files they are including) do a call/jmp to something on the stack, so I've added the .note.GNU-Stack section to the files: diff --git a/x86/bios/bios-rom.S b/x86/bios/bios-rom.S index 3269ce9793ae..571029fc157e 100644 --- a/x86/bios/bios-rom.S +++ b/x86/bios/bios-rom.S @@ -10,3 +10,6 @@ GLOBAL(bios_rom) .incbin "x86/bios/bios.bin" END(bios_rom) + +# Mark the stack as non-executable. +.section .note.GNU-stack,"",@progbits diff --git a/x86/bios/entry.S b/x86/bios/entry.S index 85056e9816c4..4d5bb663a25d 100644 --- a/x86/bios/entry.S +++ b/x86/bios/entry.S @@ -90,3 +90,6 @@ GLOBAL(__locals) #include "local.S" END(__locals) + +# Mark the stack as non-executable. +.section .note.GNU-stack,"",@progbits which makes the final executable have a non-executable stack. Did some very *light* testing by booting a guest, and everything looked right to me. [1] https://www.airs.com/blog/archives/518 Thanks, Alex > > Martin Radev (5): > kvmtool: Add WARN_ONCE macro > virtio: Sanitize config accesses > virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL > Makefile: Mark stack as not executable > mmio: Sanitize addr and len > > Makefile | 7 +++-- > include/kvm/util.h | 10 +++++++ > include/kvm/virtio-9p.h | 1 + > include/kvm/virtio.h | 3 ++- > mmio.c | 4 +++ > virtio/9p.c | 27 ++++++++++++++----- > virtio/balloon.c | 10 ++++++- > virtio/blk.c | 10 ++++++- > virtio/console.c | 10 ++++++- > virtio/mmio.c | 44 +++++++++++++++++++++++++----- > virtio/net.c | 12 +++++++-- > virtio/pci.c | 59 ++++++++++++++++++++++++++++++++++++++--- > virtio/rng.c | 8 +++++- > virtio/scsi.c | 10 ++++++- > virtio/vsock.c | 10 ++++++- > 15 files changed, 199 insertions(+), 26 deletions(-) > > -- > 2.25.1 > ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code 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 0 siblings, 1 reply; 24+ messages in thread From: Andre Przywara @ 2022-03-11 11:23 UTC (permalink / raw) To: Alexandru Elisei; +Cc: Martin Radev, kvm, will, julien.thierry.kdev On Thu, 10 Mar 2022 14:56:30 +0000 Alexandru Elisei <alexandru.elisei@arm.com> wrote: Hi, > Hi Martin, > > On Fri, Mar 04, 2022 at 01:10:45AM +0200, Martin Radev wrote: > > Hello everyone, > > > [..] > > The Makefile change is kept in its original form because I didn't understand > > if there is an issue with it on aarch64. > > I'll try to explain it better. According to this blogpost about executable > stacks [1], gcc marks the stack as executable automatically for assembly > (.S) files. C files have their stack mark as non-executable by default. If > any of the object files have the stack executable, then the resulting > binary also has the stack marked as executable (obviously). > > To mark the stack as non-executable in assembly files, the empty section > .note.GNU-stack must be present in the file. This is a marking to tell > the linker that the final executable does not require an executable stack. > When the linker finds this section, it will create a PT_GNU_STACK empty > segment in the final executable. This segment tells Linux to mark the stack > as non-executable when it loads the binary. Ah, many thanks for the explanation, that makes sense. > The only assembly files that kvmtool compiles into objects are the x86 > files x86/bios/entry.S and x86/bios/bios-rom.S; the other architectures are > not affected by this. I haven't found any instances where these files (and > the other files they are including) do a call/jmp to something on the > stack, so I've added the .note.GNU-Stack section to the files: Yes, looks that the same to me, actually the assembly looks more like marshalling arguments than actual code, so we should be safe. Alex, can you send this as a proper patch. It should be somewhat independent of Martin's series, code-wise, so at least it should apply and build. Cheers, Andre > > diff --git a/x86/bios/bios-rom.S b/x86/bios/bios-rom.S > index 3269ce9793ae..571029fc157e 100644 > --- a/x86/bios/bios-rom.S > +++ b/x86/bios/bios-rom.S > @@ -10,3 +10,6 @@ > GLOBAL(bios_rom) > .incbin "x86/bios/bios.bin" > END(bios_rom) > + > +# Mark the stack as non-executable. > +.section .note.GNU-stack,"",@progbits > diff --git a/x86/bios/entry.S b/x86/bios/entry.S > index 85056e9816c4..4d5bb663a25d 100644 > --- a/x86/bios/entry.S > +++ b/x86/bios/entry.S > @@ -90,3 +90,6 @@ GLOBAL(__locals) > #include "local.S" > > END(__locals) > + > +# Mark the stack as non-executable. > +.section .note.GNU-stack,"",@progbits > > which makes the final executable have a non-executable stack. Did some very > *light* testing by booting a guest, and everything looked right to me. > > [1] https://www.airs.com/blog/archives/518 > > Thanks, > Alex > > > > > Martin Radev (5): > > kvmtool: Add WARN_ONCE macro > > virtio: Sanitize config accesses > > virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL > > Makefile: Mark stack as not executable > > mmio: Sanitize addr and len > > > > Makefile | 7 +++-- > > include/kvm/util.h | 10 +++++++ > > include/kvm/virtio-9p.h | 1 + > > include/kvm/virtio.h | 3 ++- > > mmio.c | 4 +++ > > virtio/9p.c | 27 ++++++++++++++----- > > virtio/balloon.c | 10 ++++++- > > virtio/blk.c | 10 ++++++- > > virtio/console.c | 10 ++++++- > > virtio/mmio.c | 44 +++++++++++++++++++++++++----- > > virtio/net.c | 12 +++++++-- > > virtio/pci.c | 59 ++++++++++++++++++++++++++++++++++++++--- > > virtio/rng.c | 8 +++++- > > virtio/scsi.c | 10 ++++++- > > virtio/vsock.c | 10 ++++++- > > 15 files changed, 199 insertions(+), 26 deletions(-) > > > > -- > > 2.25.1 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code 2022-03-11 11:23 ` Andre Przywara @ 2022-03-14 17:11 ` Alexandru Elisei 2022-03-27 12:46 ` Martin Radev 0 siblings, 1 reply; 24+ messages in thread From: Alexandru Elisei @ 2022-03-14 17:11 UTC (permalink / raw) To: Andre Przywara; +Cc: Martin Radev, kvm, will, julien.thierry.kdev Hi, On Fri, Mar 11, 2022 at 11:23:21AM +0000, Andre Przywara wrote: > On Thu, 10 Mar 2022 14:56:30 +0000 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi, > > > Hi Martin, > > > > On Fri, Mar 04, 2022 at 01:10:45AM +0200, Martin Radev wrote: > > > Hello everyone, > > > > > [..] > > > The Makefile change is kept in its original form because I didn't understand > > > if there is an issue with it on aarch64. > > > > I'll try to explain it better. According to this blogpost about executable > > stacks [1], gcc marks the stack as executable automatically for assembly > > (.S) files. C files have their stack mark as non-executable by default. If > > any of the object files have the stack executable, then the resulting > > binary also has the stack marked as executable (obviously). > > > > To mark the stack as non-executable in assembly files, the empty section > > .note.GNU-stack must be present in the file. This is a marking to tell > > the linker that the final executable does not require an executable stack. > > When the linker finds this section, it will create a PT_GNU_STACK empty > > segment in the final executable. This segment tells Linux to mark the stack > > as non-executable when it loads the binary. > > Ah, many thanks for the explanation, that makes sense. > > > The only assembly files that kvmtool compiles into objects are the x86 > > files x86/bios/entry.S and x86/bios/bios-rom.S; the other architectures are > > not affected by this. I haven't found any instances where these files (and > > the other files they are including) do a call/jmp to something on the > > stack, so I've added the .note.GNU-Stack section to the files: > > Yes, looks that the same to me, actually the assembly looks more like > marshalling arguments than actual code, so we should be safe. > > Alex, can you send this as a proper patch. It should be somewhat > independent of Martin's series, code-wise, so at least it should apply and > build. Martin, would you like to pick up the diff and turn it into a proper patch? You don't need to credit me as the author, you can just add a Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com> tag in the commit message. Or do you want me to turn this into a patch? If I do, I'll add a Reported-by: Martin Radev <martin.b.radev@gmail.com> tag to it. I don't have a preference, I am asking because you were the first person who discovered and tried to fix this. Thanks, Alex > > Cheers, > Andre > > > > > diff --git a/x86/bios/bios-rom.S b/x86/bios/bios-rom.S > > index 3269ce9793ae..571029fc157e 100644 > > --- a/x86/bios/bios-rom.S > > +++ b/x86/bios/bios-rom.S > > @@ -10,3 +10,6 @@ > > GLOBAL(bios_rom) > > .incbin "x86/bios/bios.bin" > > END(bios_rom) > > + > > +# Mark the stack as non-executable. > > +.section .note.GNU-stack,"",@progbits > > diff --git a/x86/bios/entry.S b/x86/bios/entry.S > > index 85056e9816c4..4d5bb663a25d 100644 > > --- a/x86/bios/entry.S > > +++ b/x86/bios/entry.S > > @@ -90,3 +90,6 @@ GLOBAL(__locals) > > #include "local.S" > > > > END(__locals) > > + > > +# Mark the stack as non-executable. > > +.section .note.GNU-stack,"",@progbits > > > > which makes the final executable have a non-executable stack. Did some very > > *light* testing by booting a guest, and everything looked right to me. > > > > [1] https://www.airs.com/blog/archives/518 > > > > Thanks, > > Alex > > > > > > > > Martin Radev (5): > > > kvmtool: Add WARN_ONCE macro > > > virtio: Sanitize config accesses > > > virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL > > > Makefile: Mark stack as not executable > > > mmio: Sanitize addr and len > > > > > > Makefile | 7 +++-- > > > include/kvm/util.h | 10 +++++++ > > > include/kvm/virtio-9p.h | 1 + > > > include/kvm/virtio.h | 3 ++- > > > mmio.c | 4 +++ > > > virtio/9p.c | 27 ++++++++++++++----- > > > virtio/balloon.c | 10 ++++++- > > > virtio/blk.c | 10 ++++++- > > > virtio/console.c | 10 ++++++- > > > virtio/mmio.c | 44 +++++++++++++++++++++++++----- > > > virtio/net.c | 12 +++++++-- > > > virtio/pci.c | 59 ++++++++++++++++++++++++++++++++++++++--- > > > virtio/rng.c | 8 +++++- > > > virtio/scsi.c | 10 ++++++- > > > virtio/vsock.c | 10 ++++++- > > > 15 files changed, 199 insertions(+), 26 deletions(-) > > > > > > -- > > > 2.25.1 > > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code 2022-03-14 17:11 ` Alexandru Elisei @ 2022-03-27 12:46 ` Martin Radev 2022-04-22 10:37 ` Alexandru Elisei 0 siblings, 1 reply; 24+ messages in thread From: Martin Radev @ 2022-03-27 12:46 UTC (permalink / raw) To: Alexandru Elisei; +Cc: Andre Przywara, kvm, will, julien.thierry.kdev Thanks for the explanation and suggestion. Is this better? From 4ed0d9d3d3c39eb5b23b04227c3fee53b77d9aa5 Mon Sep 17 00:00:00 2001 From: Martin Radev <martin.b.radev@gmail.com> Date: Fri, 25 Mar 2022 23:25:42 +0200 Subject: kvmtool: Have stack be not executable on x86 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 On Mon, Mar 14, 2022 at 05:11:08PM +0000, Alexandru Elisei wrote: > Hi, > > On Fri, Mar 11, 2022 at 11:23:21AM +0000, Andre Przywara wrote: > > On Thu, 10 Mar 2022 14:56:30 +0000 > > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > > > Hi, > > > > > Hi Martin, > > > > > > On Fri, Mar 04, 2022 at 01:10:45AM +0200, Martin Radev wrote: > > > > Hello everyone, > > > > > > > [..] > > > > The Makefile change is kept in its original form because I didn't understand > > > > if there is an issue with it on aarch64. > > > > > > I'll try to explain it better. According to this blogpost about executable > > > stacks [1], gcc marks the stack as executable automatically for assembly > > > (.S) files. C files have their stack mark as non-executable by default. If > > > any of the object files have the stack executable, then the resulting > > > binary also has the stack marked as executable (obviously). > > > > > > To mark the stack as non-executable in assembly files, the empty section > > > .note.GNU-stack must be present in the file. This is a marking to tell > > > the linker that the final executable does not require an executable stack. > > > When the linker finds this section, it will create a PT_GNU_STACK empty > > > segment in the final executable. This segment tells Linux to mark the stack > > > as non-executable when it loads the binary. > > > > Ah, many thanks for the explanation, that makes sense. > > > > > The only assembly files that kvmtool compiles into objects are the x86 > > > files x86/bios/entry.S and x86/bios/bios-rom.S; the other architectures are > > > not affected by this. I haven't found any instances where these files (and > > > the other files they are including) do a call/jmp to something on the > > > stack, so I've added the .note.GNU-Stack section to the files: > > > > Yes, looks that the same to me, actually the assembly looks more like > > marshalling arguments than actual code, so we should be safe. > > > > Alex, can you send this as a proper patch. It should be somewhat > > independent of Martin's series, code-wise, so at least it should apply and > > build. > > Martin, would you like to pick up the diff and turn it into a proper patch? You > don't need to credit me as the author, you can just add a Suggested-by: > Alexandru Elisei <alexandru.elisei@arm.com> tag in the commit message. Or do you > want me to turn this into a patch? If I do, I'll add a Reported-by: Martin Radev > <martin.b.radev@gmail.com> tag to it. > > I don't have a preference, I am asking because you were the first person who > discovered and tried to fix this. > > Thanks, > Alex > > > > > Cheers, > > Andre > > > > > > > > diff --git a/x86/bios/bios-rom.S b/x86/bios/bios-rom.S > > > index 3269ce9793ae..571029fc157e 100644 > > > --- a/x86/bios/bios-rom.S > > > +++ b/x86/bios/bios-rom.S > > > @@ -10,3 +10,6 @@ > > > GLOBAL(bios_rom) > > > .incbin "x86/bios/bios.bin" > > > END(bios_rom) > > > + > > > +# Mark the stack as non-executable. > > > +.section .note.GNU-stack,"",@progbits > > > diff --git a/x86/bios/entry.S b/x86/bios/entry.S > > > index 85056e9816c4..4d5bb663a25d 100644 > > > --- a/x86/bios/entry.S > > > +++ b/x86/bios/entry.S > > > @@ -90,3 +90,6 @@ GLOBAL(__locals) > > > #include "local.S" > > > > > > END(__locals) > > > + > > > +# Mark the stack as non-executable. > > > +.section .note.GNU-stack,"",@progbits > > > > > > which makes the final executable have a non-executable stack. Did some very > > > *light* testing by booting a guest, and everything looked right to me. > > > > > > [1] https://www.airs.com/blog/archives/518 > > > > > > Thanks, > > > Alex > > > > > > > > > > > Martin Radev (5): > > > > kvmtool: Add WARN_ONCE macro > > > > virtio: Sanitize config accesses > > > > virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL > > > > Makefile: Mark stack as not executable > > > > mmio: Sanitize addr and len > > > > > > > > Makefile | 7 +++-- > > > > include/kvm/util.h | 10 +++++++ > > > > include/kvm/virtio-9p.h | 1 + > > > > include/kvm/virtio.h | 3 ++- > > > > mmio.c | 4 +++ > > > > virtio/9p.c | 27 ++++++++++++++----- > > > > virtio/balloon.c | 10 ++++++- > > > > virtio/blk.c | 10 ++++++- > > > > virtio/console.c | 10 ++++++- > > > > virtio/mmio.c | 44 +++++++++++++++++++++++++----- > > > > virtio/net.c | 12 +++++++-- > > > > virtio/pci.c | 59 ++++++++++++++++++++++++++++++++++++++--- > > > > virtio/rng.c | 8 +++++- > > > > virtio/scsi.c | 10 ++++++- > > > > virtio/vsock.c | 10 ++++++- > > > > 15 files changed, 199 insertions(+), 26 deletions(-) > > > > > > > > -- > > > > 2.25.1 > > > > > > ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code 2022-03-27 12:46 ` Martin Radev @ 2022-04-22 10:37 ` Alexandru Elisei 0 siblings, 0 replies; 24+ messages in thread From: Alexandru Elisei @ 2022-04-22 10:37 UTC (permalink / raw) To: Martin Radev; +Cc: Andre Przywara, kvm, will, julien.thierry.kdev Hi, On Sun, Mar 27, 2022 at 03:46:51PM +0300, Martin Radev wrote: > Thanks for the explanation and suggestion. > Is this better? Looks good to me, please send it as a separate patch in the next iteration of the series. Thanks, Alex > > From 4ed0d9d3d3c39eb5b23b04227c3fee53b77d9aa5 Mon Sep 17 00:00:00 2001 > From: Martin Radev <martin.b.radev@gmail.com> > Date: Fri, 25 Mar 2022 23:25:42 +0200 > Subject: kvmtool: Have stack be not executable on x86 > > 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 > > On Mon, Mar 14, 2022 at 05:11:08PM +0000, Alexandru Elisei wrote: > > Hi, > > > > On Fri, Mar 11, 2022 at 11:23:21AM +0000, Andre Przywara wrote: > > > On Thu, 10 Mar 2022 14:56:30 +0000 > > > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > > > > > Hi, > > > > > > > Hi Martin, > > > > > > > > On Fri, Mar 04, 2022 at 01:10:45AM +0200, Martin Radev wrote: > > > > > Hello everyone, > > > > > > > > > [..] > > > > > The Makefile change is kept in its original form because I didn't understand > > > > > if there is an issue with it on aarch64. > > > > > > > > I'll try to explain it better. According to this blogpost about executable > > > > stacks [1], gcc marks the stack as executable automatically for assembly > > > > (.S) files. C files have their stack mark as non-executable by default. If > > > > any of the object files have the stack executable, then the resulting > > > > binary also has the stack marked as executable (obviously). > > > > > > > > To mark the stack as non-executable in assembly files, the empty section > > > > .note.GNU-stack must be present in the file. This is a marking to tell > > > > the linker that the final executable does not require an executable stack. > > > > When the linker finds this section, it will create a PT_GNU_STACK empty > > > > segment in the final executable. This segment tells Linux to mark the stack > > > > as non-executable when it loads the binary. > > > > > > Ah, many thanks for the explanation, that makes sense. > > > > > > > The only assembly files that kvmtool compiles into objects are the x86 > > > > files x86/bios/entry.S and x86/bios/bios-rom.S; the other architectures are > > > > not affected by this. I haven't found any instances where these files (and > > > > the other files they are including) do a call/jmp to something on the > > > > stack, so I've added the .note.GNU-Stack section to the files: > > > > > > Yes, looks that the same to me, actually the assembly looks more like > > > marshalling arguments than actual code, so we should be safe. > > > > > > Alex, can you send this as a proper patch. It should be somewhat > > > independent of Martin's series, code-wise, so at least it should apply and > > > build. > > > > Martin, would you like to pick up the diff and turn it into a proper patch? You > > don't need to credit me as the author, you can just add a Suggested-by: > > Alexandru Elisei <alexandru.elisei@arm.com> tag in the commit message. Or do you > > want me to turn this into a patch? If I do, I'll add a Reported-by: Martin Radev > > <martin.b.radev@gmail.com> tag to it. > > > > I don't have a preference, I am asking because you were the first person who > > discovered and tried to fix this. > > > > Thanks, > > Alex > > > > > > > > Cheers, > > > Andre > > > > > > > > > > > diff --git a/x86/bios/bios-rom.S b/x86/bios/bios-rom.S > > > > index 3269ce9793ae..571029fc157e 100644 > > > > --- a/x86/bios/bios-rom.S > > > > +++ b/x86/bios/bios-rom.S > > > > @@ -10,3 +10,6 @@ > > > > GLOBAL(bios_rom) > > > > .incbin "x86/bios/bios.bin" > > > > END(bios_rom) > > > > + > > > > +# Mark the stack as non-executable. > > > > +.section .note.GNU-stack,"",@progbits > > > > diff --git a/x86/bios/entry.S b/x86/bios/entry.S > > > > index 85056e9816c4..4d5bb663a25d 100644 > > > > --- a/x86/bios/entry.S > > > > +++ b/x86/bios/entry.S > > > > @@ -90,3 +90,6 @@ GLOBAL(__locals) > > > > #include "local.S" > > > > > > > > END(__locals) > > > > + > > > > +# Mark the stack as non-executable. > > > > +.section .note.GNU-stack,"",@progbits > > > > > > > > which makes the final executable have a non-executable stack. Did some very > > > > *light* testing by booting a guest, and everything looked right to me. > > > > > > > > [1] https://www.airs.com/blog/archives/518 > > > > > > > > Thanks, > > > > Alex > > > > > > > > > > > > > > Martin Radev (5): > > > > > kvmtool: Add WARN_ONCE macro > > > > > virtio: Sanitize config accesses > > > > > virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL > > > > > Makefile: Mark stack as not executable > > > > > mmio: Sanitize addr and len > > > > > > > > > > Makefile | 7 +++-- > > > > > include/kvm/util.h | 10 +++++++ > > > > > include/kvm/virtio-9p.h | 1 + > > > > > include/kvm/virtio.h | 3 ++- > > > > > mmio.c | 4 +++ > > > > > virtio/9p.c | 27 ++++++++++++++----- > > > > > virtio/balloon.c | 10 ++++++- > > > > > virtio/blk.c | 10 ++++++- > > > > > virtio/console.c | 10 ++++++- > > > > > virtio/mmio.c | 44 +++++++++++++++++++++++++----- > > > > > virtio/net.c | 12 +++++++-- > > > > > virtio/pci.c | 59 ++++++++++++++++++++++++++++++++++++++--- > > > > > virtio/rng.c | 8 +++++- > > > > > virtio/scsi.c | 10 ++++++- > > > > > virtio/vsock.c | 10 ++++++- > > > > > 15 files changed, 199 insertions(+), 26 deletions(-) > > > > > > > > > > -- > > > > > 2.25.1 > > > > > > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code 2022-03-03 23:10 [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Martin Radev ` (5 preceding siblings ...) 2022-03-10 14:56 ` [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code Alexandru Elisei @ 2022-05-06 13:20 ` Will Deacon 6 siblings, 0 replies; 24+ messages in thread From: Will Deacon @ 2022-05-06 13:20 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, julien.thierry.kdev, andre.przywara, alexandru.elisei On Fri, Mar 04, 2022 at 01:10:45AM +0200, Martin Radev wrote: > Hello everyone, > > Thanks for the reviews in the first patch set. > > This is the second version of the original patch set which addresses > few found overflows in the common virtio code. Since the first version, > the following changes were made: > - the virtio_net warning patch was removed. > - a WARN_ONCE macro is added to help signal that an issue was observed, > but without polluting the log. > - a couple of improvements in sanitization and style. > - TODO comment for missing handling of multi-byte PCI accesses. > > The Makefile change is kept in its original form because I didn't understand > if there is an issue with it on aarch64. It looks like we're nearly there with this series. Martin, do you plan to spin a v3, please? Will ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH kvmtool 0/5] kvmtool: Fix few found bugs @ 2022-01-17 22:11 Martin Radev 2022-01-17 22:12 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev 0 siblings, 1 reply; 24+ messages in thread From: Martin Radev @ 2022-01-17 22:11 UTC (permalink / raw) To: kvm; +Cc: will, julien.thierry.kdev, Martin Radev In December, we hosted a CTF where one of the challenges was exploiting any "0day" bug in kvmtool [1]. Eight teams managed to find a bug and exploit it in less than 36 hours. Write-ups for exploits are available by HXP [2] and kalmarunionen [3]. Now, I'm aware that kvmtool is mostly used for KVM testing and KVM bring-up in simulation environments. But since it does get mentioned in some security- related projects [4, 5] and has a sandboxing feature, maybe it makes sense to fix these bugs. Could you please check if these patches make sense? I have not verified that these patches do not break something for these virtio drivers. Kind regards, Martin [1]: https://2021.ctf.link/internal/challenge/dd0e8826-c970-4fde-8eeb-41a9d8a86b67/ [2]: https://hxp.io/blog/87/hxp-CTF-2021-indie_vmm-writeup/ [3]: https://www.kalmarunionen.dk/writeups/2021/hxp-2021/lkvm/ [4]: https://blog.quarkslab.com/no-tears-no-fears.html [5]: https://fly.io/blog/sandboxing-and-workload-isolation/ Martin Radev (5): virtio: Sanitize config accesses virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL virtio/net: Warn if virtio_net is implicitly enabled Makefile: Mark stack as not executable mmio: Sanitize addr and len Makefile | 7 +++++-- include/kvm/virtio-9p.h | 1 + include/kvm/virtio.h | 3 ++- mmio.c | 4 ++++ virtio/9p.c | 21 ++++++++++++++++---- virtio/balloon.c | 8 +++++++- virtio/blk.c | 8 +++++++- virtio/console.c | 8 +++++++- virtio/mmio.c | 44 ++++++++++++++++++++++++++++++++++------- virtio/net.c | 11 ++++++++++- virtio/pci.c | 40 +++++++++++++++++++++++++++++++++---- virtio/rng.c | 8 +++++++- virtio/scsi.c | 8 +++++++- virtio/vsock.c | 8 +++++++- 14 files changed, 154 insertions(+), 25 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH kvmtool 5/5] mmio: Sanitize addr and len 2022-01-17 22:11 [PATCH kvmtool 0/5] kvmtool: Fix few found bugs Martin Radev @ 2022-01-17 22:12 ` Martin Radev 2022-02-01 15:34 ` Alexandru Elisei 2022-02-01 15:52 ` Andre Przywara 0 siblings, 2 replies; 24+ messages in thread From: Martin Radev @ 2022-01-17 22:12 UTC (permalink / raw) To: kvm; +Cc: will, julien.thierry.kdev, Martin Radev This patch verifies that adding the addr and length arguments from an MMIO op do not overflow. This is necessary because the arguments are controlled by the VM. The length may be set to an arbitrary value by using the rep prefix. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- mmio.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/mmio.c b/mmio.c index a6dd3aa..04d2af6 100644 --- a/mmio.c +++ b/mmio.c @@ -32,6 +32,10 @@ static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len) { struct rb_int_node *node; + /* If len is zero or if there's an overflow, the MMIO op is invalid. */ + if (len + addr <= addr) + return NULL; + node = rb_int_search_range(root, addr, addr + len); if (node == NULL) return NULL; -- 2.25.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH kvmtool 5/5] mmio: Sanitize addr and len 2022-01-17 22:12 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev @ 2022-02-01 15:34 ` Alexandru Elisei 2022-02-01 15:52 ` Andre Przywara 1 sibling, 0 replies; 24+ messages in thread From: Alexandru Elisei @ 2022-02-01 15:34 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev Hi Martin, On Tue, Jan 18, 2022 at 12:12:03AM +0200, Martin Radev wrote: > This patch verifies that adding the addr and length arguments > from an MMIO op do not overflow. This is necessary because the > arguments are controlled by the VM. The length may be set to > an arbitrary value by using the rep prefix. The Arm architecture doesn'tt have instructions to access 0 bytes of memory. By "rep prefix" you mean the x86 rep* instructions, right? > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > mmio.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/mmio.c b/mmio.c > index a6dd3aa..04d2af6 100644 > --- a/mmio.c > +++ b/mmio.c > @@ -32,6 +32,10 @@ static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len) > { > struct rb_int_node *node; > > + /* If len is zero or if there's an overflow, the MMIO op is invalid. */ > + if (len + addr <= addr) Would you mind rewriting the left side of the inequality to addr + len to match the argument to rb_int_search_range() below? Otherwise looks good: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex > + return NULL; > + > node = rb_int_search_range(root, addr, addr + len); > if (node == NULL) > return NULL; > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH kvmtool 5/5] mmio: Sanitize addr and len 2022-01-17 22:12 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev 2022-02-01 15:34 ` Alexandru Elisei @ 2022-02-01 15:52 ` Andre Przywara 1 sibling, 0 replies; 24+ messages in thread From: Andre Przywara @ 2022-02-01 15:52 UTC (permalink / raw) To: Martin Radev Cc: kvm, will, julien.thierry.kdev, Alexandru Elisei, Marc Zyngier, Jean-Philippe Brucker On Tue, 18 Jan 2022 00:12:03 +0200 Martin Radev <martin.b.radev@gmail.com> wrote: > This patch verifies that adding the addr and length arguments > from an MMIO op do not overflow. This is necessary because the > arguments are controlled by the VM. The length may be set to > an arbitrary value by using the rep prefix. Mmh, interesting, so does the kernel collate this into one "giant" KVM_EXIT_MMIO with an arbitrary length? I wonder if there are assumptions in the MMIO code of len never being bigger than say 16. On ARM/ARM64 we probably never see len being bigger than 8 on those exits. But the check is certainly fine anyway... > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> Reviewed-by: Andre Przywara <andre.przywara@arm.com> Thanks, Andre > --- > mmio.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/mmio.c b/mmio.c > index a6dd3aa..04d2af6 100644 > --- a/mmio.c > +++ b/mmio.c > @@ -32,6 +32,10 @@ static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len) > { > struct rb_int_node *node; > > + /* If len is zero or if there's an overflow, the MMIO op is invalid. */ > + if (len + addr <= addr) > + return NULL; > + > node = rb_int_search_range(root, addr, addr + len); > if (node == NULL) > return NULL; ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-05-06 13:20 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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 -- strict thread matches above, loose matches on Subject: below -- 2022-01-17 22:11 [PATCH kvmtool 0/5] kvmtool: Fix few found bugs Martin Radev 2022-01-17 22:12 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev 2022-02-01 15:34 ` Alexandru Elisei 2022-02-01 15:52 ` Andre Przywara
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.