* [PATCH kvmtool 0/5] kvmtool: Fix few found bugs @ 2022-01-17 22:11 Martin Radev 2022-01-17 22:11 ` [PATCH kvmtool 1/5] virtio: Sanitize config accesses Martin Radev ` (4 more replies) 0 siblings, 5 replies; 20+ 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] 20+ messages in thread
* [PATCH kvmtool 1/5] virtio: Sanitize config accesses 2022-01-17 22:11 [PATCH kvmtool 0/5] kvmtool: Fix few found bugs Martin Radev @ 2022-01-17 22:11 ` Martin Radev 2022-02-01 14:55 ` Andre Przywara 2022-02-01 15:27 ` Alexandru Elisei 2022-01-17 22:12 ` [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev ` (3 subsequent siblings) 4 siblings, 2 replies; 20+ messages in thread From: Martin Radev @ 2022-01-17 22:11 UTC (permalink / raw) To: kvm; +Cc: will, julien.thierry.kdev, Martin Radev The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows. This patch sanitizes this operation by using the newly added virtio op get_config_size. Any access which goes beyond the config structure's size is prevented and a failure is returned. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- include/kvm/virtio-9p.h | 1 + include/kvm/virtio.h | 1 + virtio/9p.c | 19 ++++++++++++++++--- virtio/balloon.c | 6 ++++++ virtio/blk.c | 6 ++++++ virtio/console.c | 6 ++++++ virtio/mmio.c | 19 +++++++++++++++---- virtio/net.c | 6 ++++++ virtio/pci.c | 19 ++++++++++++++++++- virtio/rng.c | 6 ++++++ virtio/scsi.c | 6 ++++++ virtio/vsock.c | 6 ++++++ 12 files changed, 93 insertions(+), 8 deletions(-) diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h index 3ea7698..77c5062 100644 --- a/include/kvm/virtio-9p.h +++ b/include/kvm/virtio-9p.h @@ -44,6 +44,7 @@ struct p9_dev { struct virtio_device vdev; struct rb_root fids; + size_t config_size; struct virtio_9p_config *config; u32 features; diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h index 3a311f5..3880e74 100644 --- a/include/kvm/virtio.h +++ b/include/kvm/virtio.h @@ -184,6 +184,7 @@ struct virtio_device { struct virtio_ops { u8 *(*get_config)(struct kvm *kvm, void *dev); + size_t (*get_config_size)(struct kvm *kvm, void *dev); u32 (*get_host_features)(struct kvm *kvm, void *dev); void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); int (*get_vq_count)(struct kvm *kvm, void *dev); diff --git a/virtio/9p.c b/virtio/9p.c index b78f2b3..89bec5e 100644 --- a/virtio/9p.c +++ b/virtio/9p.c @@ -1375,6 +1375,12 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(p9dev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + struct p9_dev *p9dev = dev; + return p9dev->config_size; +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 1 << VIRTIO_9P_MOUNT_TAG; @@ -1469,6 +1475,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) struct virtio_ops p9_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq, @@ -1569,6 +1576,8 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) { struct p9_dev *p9dev; int err = 0; + size_t tag_name_length = 0; + size_t config_size = 0; p9dev = calloc(1, sizeof(*p9dev)); if (!p9dev) @@ -1577,22 +1586,26 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) if (!tag_name) tag_name = VIRTIO_9P_DEFAULT_TAG; - p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1); + tag_name_length = strlen(tag_name); + config_size = sizeof(*p9dev->config) + tag_name_length + 1; + + p9dev->config = calloc(1, config_size); if (p9dev->config == NULL) { err = -ENOMEM; goto free_p9dev; } + p9dev->config_size = config_size; strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir)); p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00'; - p9dev->config->tag_len = strlen(tag_name); + p9dev->config->tag_len = tag_name_length; if (p9dev->config->tag_len > MAX_TAG_LEN) { err = -EINVAL; goto free_p9dev_config; } - memcpy(&p9dev->config->tag, tag_name, strlen(tag_name)); + memcpy(&p9dev->config->tag, tag_name, tag_name_length); list_add(&p9dev->list, &devs); diff --git a/virtio/balloon.c b/virtio/balloon.c index 8e8803f..233a3a5 100644 --- a/virtio/balloon.c +++ b/virtio/balloon.c @@ -181,6 +181,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&bdev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + return sizeof(struct virtio_balloon_config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 1 << VIRTIO_BALLOON_F_STATS_VQ; @@ -251,6 +256,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) struct virtio_ops bln_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq, diff --git a/virtio/blk.c b/virtio/blk.c index 4d02d10..9164b51 100644 --- a/virtio/blk.c +++ b/virtio/blk.c @@ -146,6 +146,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&bdev->blk_config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + return sizeof(struct virtio_blk_config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { struct blk_dev *bdev = dev; @@ -291,6 +296,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops blk_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .get_vq_count = get_vq_count, diff --git a/virtio/console.c b/virtio/console.c index e0b98df..00bafa2 100644 --- a/virtio/console.c +++ b/virtio/console.c @@ -121,6 +121,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&cdev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + return sizeof(struct virtio_console_config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 0; @@ -216,6 +221,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops con_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .get_vq_count = get_vq_count, diff --git a/virtio/mmio.c b/virtio/mmio.c index 875a288..32bba17 100644 --- a/virtio/mmio.c +++ b/virtio/mmio.c @@ -103,15 +103,26 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu, u8 is_write, struct virtio_device *vdev) { struct virtio_mmio *vmmio = vdev->virtio; + u8 *config_aperture = NULL; + u32 config_aperture_size = 0; u32 i; + config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev); + /* The cast here is safe because get_config_size will always fit in 32 bits. */ + config_aperture_size = (u32)vdev->ops->get_config_size(vmmio->kvm, vmmio->dev); + + /* Reduce length to no more than the config size to avoid buffer overflows. */ + if (config_aperture_size < len) { + pr_warning("Length (%u) goes beyond config size (%u).\n", + len, config_aperture_size); + len = config_aperture_size; + } + for (i = 0; i < len; i++) { if (is_write) - vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] = - *(u8 *)data + i; + config_aperture[addr + i] = *(u8 *)data + i; else - data[i] = vdev->ops->get_config(vmmio->kvm, - vmmio->dev)[addr + i]; + data[i] = config_aperture[addr+i]; } } diff --git a/virtio/net.c b/virtio/net.c index 1ee3c19..75d9ae5 100644 --- a/virtio/net.c +++ b/virtio/net.c @@ -480,6 +480,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&ndev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + return sizeof(struct virtio_net_config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { u32 features; @@ -757,6 +762,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops net_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .get_vq_count = get_vq_count, diff --git a/virtio/pci.c b/virtio/pci.c index 4108529..50fdaa4 100644 --- a/virtio/pci.c +++ b/virtio/pci.c @@ -136,7 +136,15 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device * return true; } else if (type == VIRTIO_PCI_O_CONFIG) { u8 cfg; - + size_t config_size; + + config_size = vdev->ops->get_config_size(kvm, vpci->dev); + if (config_offset >= config_size) { + /* Access goes beyond the config size, so return failure. */ + pr_warning("Config access offset (%u) is beyond config size (%zu)\n", + config_offset, config_size); + return false; + } cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset]; ioport__write8(data, cfg); return true; @@ -276,6 +284,15 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device return true; } else if (type == VIRTIO_PCI_O_CONFIG) { + size_t config_size; + + config_size = vdev->ops->get_config_size(kvm, vpci->dev); + if (config_offset >= config_size) { + /* Access goes beyond the config size, so return failure. */ + pr_warning("Config access offset (%u) is beyond config size (%zu)\n", + config_offset, config_size); + return false; + } vdev->ops->get_config(kvm, vpci->dev)[config_offset] = *(u8 *)data; return true; diff --git a/virtio/rng.c b/virtio/rng.c index 78eaa64..c7835a0 100644 --- a/virtio/rng.c +++ b/virtio/rng.c @@ -47,6 +47,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) return 0; } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + return 0; +} + static u32 get_host_features(struct kvm *kvm, void *dev) { /* Unused */ @@ -149,6 +154,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops rng_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq, diff --git a/virtio/scsi.c b/virtio/scsi.c index 16a86cb..37418f8 100644 --- a/virtio/scsi.c +++ b/virtio/scsi.c @@ -38,6 +38,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&sdev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + return sizeof(struct virtio_scsi_config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 1UL << VIRTIO_RING_F_EVENT_IDX | @@ -176,6 +181,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops scsi_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq, diff --git a/virtio/vsock.c b/virtio/vsock.c index 5b99838..2df04d7 100644 --- a/virtio/vsock.c +++ b/virtio/vsock.c @@ -41,6 +41,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&vdev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + return sizeof(struct virtio_vsock_config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 1UL << VIRTIO_RING_F_EVENT_IDX @@ -204,6 +209,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops vsock_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq, -- 2.25.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool 1/5] virtio: Sanitize config accesses 2022-01-17 22:11 ` [PATCH kvmtool 1/5] virtio: Sanitize config accesses Martin Radev @ 2022-02-01 14:55 ` Andre Przywara 2022-02-01 15:27 ` Alexandru Elisei 1 sibling, 0 replies; 20+ messages in thread From: Andre Przywara @ 2022-02-01 14:55 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:11:59 +0200 Martin Radev <martin.b.radev@gmail.com> wrote: Hi Martin, many thanks for sending this! In general that looks good to me, just some minor things below. > The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows. > This patch sanitizes this operation by using the newly added virtio op > get_config_size. Any access which goes beyond the config structure's > size is prevented and a failure is returned. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > include/kvm/virtio-9p.h | 1 + > include/kvm/virtio.h | 1 + > virtio/9p.c | 19 ++++++++++++++++--- > virtio/balloon.c | 6 ++++++ > virtio/blk.c | 6 ++++++ > virtio/console.c | 6 ++++++ > virtio/mmio.c | 19 +++++++++++++++---- > virtio/net.c | 6 ++++++ > virtio/pci.c | 19 ++++++++++++++++++- > virtio/rng.c | 6 ++++++ > virtio/scsi.c | 6 ++++++ > virtio/vsock.c | 6 ++++++ > 12 files changed, 93 insertions(+), 8 deletions(-) > > diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h > index 3ea7698..77c5062 100644 > --- a/include/kvm/virtio-9p.h > +++ b/include/kvm/virtio-9p.h > @@ -44,6 +44,7 @@ struct p9_dev { > struct virtio_device vdev; > struct rb_root fids; > > + size_t config_size; > struct virtio_9p_config *config; > u32 features; > > diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h > index 3a311f5..3880e74 100644 > --- a/include/kvm/virtio.h > +++ b/include/kvm/virtio.h > @@ -184,6 +184,7 @@ struct virtio_device { > > struct virtio_ops { > u8 *(*get_config)(struct kvm *kvm, void *dev); > + size_t (*get_config_size)(struct kvm *kvm, void *dev); > u32 (*get_host_features)(struct kvm *kvm, void *dev); > void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); > int (*get_vq_count)(struct kvm *kvm, void *dev); > diff --git a/virtio/9p.c b/virtio/9p.c > index b78f2b3..89bec5e 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -1375,6 +1375,12 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(p9dev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + struct p9_dev *p9dev = dev; I think it's customary to have an empty line after variable declarations. > + return p9dev->config_size; > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1 << VIRTIO_9P_MOUNT_TAG; > @@ -1469,6 +1475,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > struct virtio_ops p9_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > @@ -1569,6 +1576,8 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) > { > struct p9_dev *p9dev; > int err = 0; > + size_t tag_name_length = 0; > + size_t config_size = 0; You should not need to initialise them here, as they are unconditionally set by the code below. Any premature use would then be spotted by the compiler. > > p9dev = calloc(1, sizeof(*p9dev)); > if (!p9dev) > @@ -1577,22 +1586,26 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) > if (!tag_name) > tag_name = VIRTIO_9P_DEFAULT_TAG; > > - p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1); > + tag_name_length = strlen(tag_name); > + config_size = sizeof(*p9dev->config) + tag_name_length + 1; > + > + p9dev->config = calloc(1, config_size); > if (p9dev->config == NULL) { > err = -ENOMEM; > goto free_p9dev; > } > + p9dev->config_size = config_size; > > strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir)); > p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00'; > > - p9dev->config->tag_len = strlen(tag_name); > + p9dev->config->tag_len = tag_name_length; > if (p9dev->config->tag_len > MAX_TAG_LEN) { > err = -EINVAL; > goto free_p9dev_config; > } > > - memcpy(&p9dev->config->tag, tag_name, strlen(tag_name)); > + memcpy(&p9dev->config->tag, tag_name, tag_name_length); > > list_add(&p9dev->list, &devs); > > diff --git a/virtio/balloon.c b/virtio/balloon.c > index 8e8803f..233a3a5 100644 > --- a/virtio/balloon.c > +++ b/virtio/balloon.c > @@ -181,6 +181,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&bdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_balloon_config); I wonder if we should return the size of the variable instead of the type, which is more future proof: struct bln_dev *bdev = dev; return sizeof bln_dev->config; (same for all the other occasions below) > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1 << VIRTIO_BALLOON_F_STATS_VQ; > @@ -251,6 +256,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > struct virtio_ops bln_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > diff --git a/virtio/blk.c b/virtio/blk.c > index 4d02d10..9164b51 100644 > --- a/virtio/blk.c > +++ b/virtio/blk.c > @@ -146,6 +146,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&bdev->blk_config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_blk_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > struct blk_dev *bdev = dev; > @@ -291,6 +296,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops blk_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .get_vq_count = get_vq_count, > diff --git a/virtio/console.c b/virtio/console.c > index e0b98df..00bafa2 100644 > --- a/virtio/console.c > +++ b/virtio/console.c > @@ -121,6 +121,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&cdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_console_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 0; > @@ -216,6 +221,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops con_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .get_vq_count = get_vq_count, > diff --git a/virtio/mmio.c b/virtio/mmio.c > index 875a288..32bba17 100644 > --- a/virtio/mmio.c > +++ b/virtio/mmio.c > @@ -103,15 +103,26 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu, > u8 is_write, struct virtio_device *vdev) > { > struct virtio_mmio *vmmio = vdev->virtio; > + u8 *config_aperture = NULL; > + u32 config_aperture_size = 0; Why is this a u32? And again, no need to initialise here, actually you can pull in the next two lines to initialise them properly right away. > u32 i; > > + config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev); > + /* The cast here is safe because get_config_size will always fit in 32 bits. */ > + config_aperture_size = (u32)vdev->ops->get_config_size(vmmio->kvm, vmmio->dev); Why the cast in the first place? Can't you just leave the variable at size_t, and let the compiler deal with the promotion below? > + > + /* Reduce length to no more than the config size to avoid buffer overflows. */ > + if (config_aperture_size < len) { > + pr_warning("Length (%u) goes beyond config size (%u).\n", > + len, config_aperture_size); So I appreciate the warning, but I wonder if this is too much, or should be rate limited, otherwise we allow the guest to flood the VMM's terminal. Is there any good practices for this problem? For kernel printk's we definitely don't allow this. + len = config_aperture_size; This should be safe in any case (even with different variable sizes), since you check that above. Or am I missing something? > + } > + > for (i = 0; i < len; i++) { > if (is_write) > - vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] = > - *(u8 *)data + i; > + config_aperture[addr + i] = *(u8 *)data + i; > else > - data[i] = vdev->ops->get_config(vmmio->kvm, > - vmmio->dev)[addr + i]; > + data[i] = config_aperture[addr+i]; Nit: addr + i > } > } > > diff --git a/virtio/net.c b/virtio/net.c > index 1ee3c19..75d9ae5 100644 > --- a/virtio/net.c > +++ b/virtio/net.c > @@ -480,6 +480,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&ndev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_net_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > u32 features; > @@ -757,6 +762,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops net_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .get_vq_count = get_vq_count, > diff --git a/virtio/pci.c b/virtio/pci.c > index 4108529..50fdaa4 100644 > --- a/virtio/pci.c > +++ b/virtio/pci.c > @@ -136,7 +136,15 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device * > return true; > } else if (type == VIRTIO_PCI_O_CONFIG) { > u8 cfg; > - > + size_t config_size; > + > + config_size = vdev->ops->get_config_size(kvm, vpci->dev); > + if (config_offset >= config_size) { > + /* Access goes beyond the config size, so return failure. */ > + pr_warning("Config access offset (%u) is beyond config size (%zu)\n", > + config_offset, config_size); Again, do we always want to warn on the console? Another alternative would be to kill the guest at this point, but this is probably too much. Cheers, Andre > + return false; > + } > cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset]; > ioport__write8(data, cfg); > return true; > @@ -276,6 +284,15 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device > > return true; > } else if (type == VIRTIO_PCI_O_CONFIG) { > + size_t config_size; > + > + config_size = vdev->ops->get_config_size(kvm, vpci->dev); > + if (config_offset >= config_size) { > + /* Access goes beyond the config size, so return failure. */ > + pr_warning("Config access offset (%u) is beyond config size (%zu)\n", > + config_offset, config_size); > + return false; > + } > vdev->ops->get_config(kvm, vpci->dev)[config_offset] = *(u8 *)data; > > return true; > diff --git a/virtio/rng.c b/virtio/rng.c > index 78eaa64..c7835a0 100644 > --- a/virtio/rng.c > +++ b/virtio/rng.c > @@ -47,6 +47,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return 0; > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return 0; > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > /* Unused */ > @@ -149,6 +154,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops rng_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > diff --git a/virtio/scsi.c b/virtio/scsi.c > index 16a86cb..37418f8 100644 > --- a/virtio/scsi.c > +++ b/virtio/scsi.c > @@ -38,6 +38,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&sdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_scsi_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1UL << VIRTIO_RING_F_EVENT_IDX | > @@ -176,6 +181,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops scsi_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > diff --git a/virtio/vsock.c b/virtio/vsock.c > index 5b99838..2df04d7 100644 > --- a/virtio/vsock.c > +++ b/virtio/vsock.c > @@ -41,6 +41,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&vdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_vsock_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1UL << VIRTIO_RING_F_EVENT_IDX > @@ -204,6 +209,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops vsock_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool 1/5] virtio: Sanitize config accesses 2022-01-17 22:11 ` [PATCH kvmtool 1/5] virtio: Sanitize config accesses Martin Radev 2022-02-01 14:55 ` Andre Przywara @ 2022-02-01 15:27 ` Alexandru Elisei 1 sibling, 0 replies; 20+ messages in thread From: Alexandru Elisei @ 2022-02-01 15:27 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev Hi Martin, I'm on shaky ground when it comes to the virtio spec, but I'll do my best to review your patch. On Tue, Jan 18, 2022 at 12:11:59AM +0200, Martin Radev wrote: > The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows. > This patch sanitizes this operation by using the newly added virtio op > get_config_size. Any access which goes beyond the config structure's > size is prevented and a failure is returned. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > include/kvm/virtio-9p.h | 1 + > include/kvm/virtio.h | 1 + > virtio/9p.c | 19 ++++++++++++++++--- > virtio/balloon.c | 6 ++++++ > virtio/blk.c | 6 ++++++ > virtio/console.c | 6 ++++++ > virtio/mmio.c | 19 +++++++++++++++---- > virtio/net.c | 6 ++++++ > virtio/pci.c | 19 ++++++++++++++++++- > virtio/rng.c | 6 ++++++ > virtio/scsi.c | 6 ++++++ > virtio/vsock.c | 6 ++++++ > 12 files changed, 93 insertions(+), 8 deletions(-) > > diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h > index 3ea7698..77c5062 100644 > --- a/include/kvm/virtio-9p.h > +++ b/include/kvm/virtio-9p.h > @@ -44,6 +44,7 @@ struct p9_dev { > struct virtio_device vdev; > struct rb_root fids; > > + size_t config_size; > struct virtio_9p_config *config; > u32 features; > > diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h > index 3a311f5..3880e74 100644 > --- a/include/kvm/virtio.h > +++ b/include/kvm/virtio.h > @@ -184,6 +184,7 @@ struct virtio_device { > > struct virtio_ops { > u8 *(*get_config)(struct kvm *kvm, void *dev); > + size_t (*get_config_size)(struct kvm *kvm, void *dev); > u32 (*get_host_features)(struct kvm *kvm, void *dev); > void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); > int (*get_vq_count)(struct kvm *kvm, void *dev); > diff --git a/virtio/9p.c b/virtio/9p.c > index b78f2b3..89bec5e 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -1375,6 +1375,12 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(p9dev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + struct p9_dev *p9dev = dev; > + return p9dev->config_size; > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1 << VIRTIO_9P_MOUNT_TAG; > @@ -1469,6 +1475,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > struct virtio_ops p9_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > @@ -1569,6 +1576,8 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) > { > struct p9_dev *p9dev; > int err = 0; > + size_t tag_name_length = 0; > + size_t config_size = 0; > > p9dev = calloc(1, sizeof(*p9dev)); > if (!p9dev) > @@ -1577,22 +1586,26 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) > if (!tag_name) > tag_name = VIRTIO_9P_DEFAULT_TAG; > > - p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1); > + tag_name_length = strlen(tag_name); > + config_size = sizeof(*p9dev->config) + tag_name_length + 1; > + > + p9dev->config = calloc(1, config_size); > if (p9dev->config == NULL) { > err = -ENOMEM; > goto free_p9dev; > } > + p9dev->config_size = config_size; > > strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir)); > p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00'; > > - p9dev->config->tag_len = strlen(tag_name); > + p9dev->config->tag_len = tag_name_length; > if (p9dev->config->tag_len > MAX_TAG_LEN) { > err = -EINVAL; > goto free_p9dev_config; > } > > - memcpy(&p9dev->config->tag, tag_name, strlen(tag_name)); > + memcpy(&p9dev->config->tag, tag_name, tag_name_length); > > list_add(&p9dev->list, &devs); > > diff --git a/virtio/balloon.c b/virtio/balloon.c > index 8e8803f..233a3a5 100644 > --- a/virtio/balloon.c > +++ b/virtio/balloon.c > @@ -181,6 +181,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&bdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_balloon_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1 << VIRTIO_BALLOON_F_STATS_VQ; > @@ -251,6 +256,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > struct virtio_ops bln_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > diff --git a/virtio/blk.c b/virtio/blk.c > index 4d02d10..9164b51 100644 > --- a/virtio/blk.c > +++ b/virtio/blk.c > @@ -146,6 +146,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&bdev->blk_config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_blk_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > struct blk_dev *bdev = dev; > @@ -291,6 +296,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops blk_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .get_vq_count = get_vq_count, > diff --git a/virtio/console.c b/virtio/console.c > index e0b98df..00bafa2 100644 > --- a/virtio/console.c > +++ b/virtio/console.c > @@ -121,6 +121,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&cdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_console_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 0; > @@ -216,6 +221,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops con_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .get_vq_count = get_vq_count, > diff --git a/virtio/mmio.c b/virtio/mmio.c > index 875a288..32bba17 100644 > --- a/virtio/mmio.c > +++ b/virtio/mmio.c > @@ -103,15 +103,26 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu, > u8 is_write, struct virtio_device *vdev) > { > struct virtio_mmio *vmmio = vdev->virtio; > + u8 *config_aperture = NULL; > + u32 config_aperture_size = 0; > u32 i; > > + config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev); > + /* The cast here is safe because get_config_size will always fit in 32 bits. */ > + config_aperture_size = (u32)vdev->ops->get_config_size(vmmio->kvm, vmmio->dev); > + > + /* Reduce length to no more than the config size to avoid buffer overflows. */ > + if (config_aperture_size < len) { > + pr_warning("Length (%u) goes beyond config size (%u).\n", > + len, config_aperture_size); > + len = config_aperture_size; > + } I think the case where addr is beyong the config region for the device isn't handled (it is handled in the case of the PCI transport). I think the function should return early if that's the case. > + > for (i = 0; i < len; i++) { > if (is_write) > - vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] = > - *(u8 *)data + i; > + config_aperture[addr + i] = *(u8 *)data + i; > else > - data[i] = vdev->ops->get_config(vmmio->kvm, > - vmmio->dev)[addr + i]; > + data[i] = config_aperture[addr+i]; Nitpick: it should be addr + i (notice the spaces) to be consistent with the rest of the file. > } > } > > diff --git a/virtio/net.c b/virtio/net.c > index 1ee3c19..75d9ae5 100644 > --- a/virtio/net.c > +++ b/virtio/net.c > @@ -480,6 +480,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&ndev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_net_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > u32 features; > @@ -757,6 +762,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops net_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .get_vq_count = get_vq_count, > diff --git a/virtio/pci.c b/virtio/pci.c > index 4108529..50fdaa4 100644 > --- a/virtio/pci.c > +++ b/virtio/pci.c > @@ -136,7 +136,15 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device * > return true; > } else if (type == VIRTIO_PCI_O_CONFIG) { > u8 cfg; > - > + size_t config_size; > + > + config_size = vdev->ops->get_config_size(kvm, vpci->dev); > + if (config_offset >= config_size) { What happens when offset + size > vdev->ops.get_config() + config_size? This case is handled in the mmio transport case, but not here. More details below. > + /* Access goes beyond the config size, so return failure. */ > + pr_warning("Config access offset (%u) is beyond config size (%zu)\n", > + config_offset, config_size); > + return false; > + } > cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset]; > ioport__write8(data, cfg); I think this is wrong. Looking at struct virtio_net_config, there are fields larger than a byte and the read here doesn't take into account the size parameter of the function. IMO, the function should be fixed to read the correct size before doing any boundary checks. This is what virtio-v1.0 has to say about transitional devices: "When using the legacy interface the driver MAY access the device-specific configuration region using any width accesses, and a transitional device MUST present driver with the same results as when accessed using the “natural” access method (i.e. 32-bit accesses for 32-bit fields, etc)". Is there something that I'm missing here? Same thing with virtio_pci__specific_data_out() below, it ignores the size parameter. Thanks, Alex > return true; > @@ -276,6 +284,15 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device > > return true; > } else if (type == VIRTIO_PCI_O_CONFIG) { > + size_t config_size; > + > + config_size = vdev->ops->get_config_size(kvm, vpci->dev); > + if (config_offset >= config_size) { > + /* Access goes beyond the config size, so return failure. */ > + pr_warning("Config access offset (%u) is beyond config size (%zu)\n", > + config_offset, config_size); > + return false; > + } > vdev->ops->get_config(kvm, vpci->dev)[config_offset] = *(u8 *)data; > > return true; > diff --git a/virtio/rng.c b/virtio/rng.c > index 78eaa64..c7835a0 100644 > --- a/virtio/rng.c > +++ b/virtio/rng.c > @@ -47,6 +47,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return 0; > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return 0; > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > /* Unused */ > @@ -149,6 +154,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops rng_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > diff --git a/virtio/scsi.c b/virtio/scsi.c > index 16a86cb..37418f8 100644 > --- a/virtio/scsi.c > +++ b/virtio/scsi.c > @@ -38,6 +38,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&sdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_scsi_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1UL << VIRTIO_RING_F_EVENT_IDX | > @@ -176,6 +181,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops scsi_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > diff --git a/virtio/vsock.c b/virtio/vsock.c > index 5b99838..2df04d7 100644 > --- a/virtio/vsock.c > +++ b/virtio/vsock.c > @@ -41,6 +41,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&vdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return sizeof(struct virtio_vsock_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1UL << VIRTIO_RING_F_EVENT_IDX > @@ -204,6 +209,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops vsock_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL 2022-01-17 22:11 [PATCH kvmtool 0/5] kvmtool: Fix few found bugs Martin Radev 2022-01-17 22:11 ` [PATCH kvmtool 1/5] virtio: Sanitize config accesses Martin Radev @ 2022-01-17 22:12 ` Martin Radev 2022-02-01 14:57 ` Andre Przywara 2022-02-01 15:28 ` Alexandru Elisei 2022-01-17 22:12 ` [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled Martin Radev ` (2 subsequent siblings) 4 siblings, 2 replies; 20+ 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 checks for overflows in QUEUE_NOTIFY and QUEUE_SEL in the PCI and MMIO operation handling paths. Further, the return value type of get_vq_count is changed from int to u32 since negative doesn't carry any semantic meaning. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- include/kvm/virtio.h | 2 +- virtio/9p.c | 2 +- virtio/balloon.c | 2 +- virtio/blk.c | 2 +- virtio/console.c | 2 +- virtio/mmio.c | 25 ++++++++++++++++++++++--- virtio/net.c | 2 +- virtio/pci.c | 21 ++++++++++++++++++--- virtio/rng.c | 2 +- virtio/scsi.c | 2 +- virtio/vsock.c | 2 +- 11 files changed, 49 insertions(+), 15 deletions(-) diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h index 3880e74..40f2a6d 100644 --- a/include/kvm/virtio.h +++ b/include/kvm/virtio.h @@ -187,7 +187,7 @@ struct virtio_ops { size_t (*get_config_size)(struct kvm *kvm, void *dev); u32 (*get_host_features)(struct kvm *kvm, void *dev); void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); - int (*get_vq_count)(struct kvm *kvm, void *dev); + u32 (*get_vq_count)(struct kvm *kvm, void *dev); int (*init_vq)(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align, u32 pfn); void (*exit_vq)(struct kvm *kvm, void *dev, u32 vq); diff --git a/virtio/9p.c b/virtio/9p.c index 89bec5e..8f1fc1f 100644 --- a/virtio/9p.c +++ b/virtio/9p.c @@ -1468,7 +1468,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static u32 get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/balloon.c b/virtio/balloon.c index 233a3a5..de3882e 100644 --- a/virtio/balloon.c +++ b/virtio/balloon.c @@ -249,7 +249,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static u32 get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/blk.c b/virtio/blk.c index 9164b51..46918a4 100644 --- a/virtio/blk.c +++ b/virtio/blk.c @@ -289,7 +289,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static u32 get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/console.c b/virtio/console.c index 00bafa2..84466d0 100644 --- a/virtio/console.c +++ b/virtio/console.c @@ -214,7 +214,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static u32 get_vq_count(struct kvm *kvm, void *dev) { return VIRTIO_CONSOLE_NUM_QUEUES; } diff --git a/virtio/mmio.c b/virtio/mmio.c index 32bba17..fd9a411 100644 --- a/virtio/mmio.c +++ b/virtio/mmio.c @@ -171,14 +171,26 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, struct virtio_mmio *vmmio = vdev->virtio; struct kvm *kvm = vmmio->kvm; u32 val = 0; + u32 vq_count = 0; + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); switch (addr) { case VIRTIO_MMIO_HOST_FEATURES_SEL: case VIRTIO_MMIO_GUEST_FEATURES_SEL: - case VIRTIO_MMIO_QUEUE_SEL: val = ioport__read32(data); *(u32 *)(((void *)&vmmio->hdr) + addr) = val; break; + case VIRTIO_MMIO_QUEUE_SEL: + { + val = ioport__read32(data); + if (val >= vq_count) { + pr_warning("QUEUE_SEL value (%u) is larger than VQ count (%u)\n", + val, vq_count); + break; + } + *(u32 *)(((void *)&vmmio->hdr) + addr) = val; + break; + } case VIRTIO_MMIO_STATUS: vmmio->hdr.status = ioport__read32(data); if (!vmmio->hdr.status) /* Sample endianness on reset */ @@ -222,6 +234,11 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, break; case VIRTIO_MMIO_QUEUE_NOTIFY: val = ioport__read32(data); + if (val > vq_count) { + pr_warning("QUEUE_NOTIFY value (%u) is larger than VQ count (%u)\n", + val, vq_count); + break; + } vdev->ops->notify_vq(vmmio->kvm, vmmio->dev, val); break; case VIRTIO_MMIO_INTERRUPT_ACK: @@ -341,10 +358,12 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev, int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev) { - int vq; + u32 vq; struct virtio_mmio *vmmio = vdev->virtio; + u32 vq_count; - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++) + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); + for (vq = 0; vq < vq_count; vq++) virtio_mmio_exit_vq(kvm, vdev, vq); return 0; diff --git a/virtio/net.c b/virtio/net.c index 75d9ae5..9a25bfa 100644 --- a/virtio/net.c +++ b/virtio/net.c @@ -753,7 +753,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static u32 get_vq_count(struct kvm *kvm, void *dev) { struct net_dev *ndev = dev; diff --git a/virtio/pci.c b/virtio/pci.c index 50fdaa4..60ae2cb 100644 --- a/virtio/pci.c +++ b/virtio/pci.c @@ -308,9 +308,11 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde struct virtio_pci *vpci; struct kvm *kvm; u32 val; + u32 vq_count; kvm = vcpu->kvm; vpci = vdev->virtio; + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); switch (offset) { case VIRTIO_PCI_GUEST_FEATURES: @@ -330,10 +332,21 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde } break; case VIRTIO_PCI_QUEUE_SEL: - vpci->queue_selector = ioport__read16(data); + val = ioport__read16(data); + if (val >= vq_count) { + pr_warning("QUEUE_SEL value (%u) is larger than VQ count (%u)\n", + val, vq_count); + return false; + } + vpci->queue_selector = val; break; case VIRTIO_PCI_QUEUE_NOTIFY: val = ioport__read16(data); + if (val >= vq_count) { + pr_warning("QUEUE_SEL value (%u) is larger than VQ count (%u)\n", + val, vq_count); + return false; + } vdev->ops->notify_vq(kvm, vpci->dev, val); break; case VIRTIO_PCI_STATUS: @@ -626,10 +639,12 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev, int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev) { - int vq; + u32 vq; struct virtio_pci *vpci = vdev->virtio; + u32 vq_count; - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++) + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); + for (vq = 0; vq < vq_count; vq++) virtio_pci_exit_vq(kvm, vdev, vq); return 0; diff --git a/virtio/rng.c b/virtio/rng.c index c7835a0..d9b9e68 100644 --- a/virtio/rng.c +++ b/virtio/rng.c @@ -147,7 +147,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static u32 get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/scsi.c b/virtio/scsi.c index 37418f8..cdf553d 100644 --- a/virtio/scsi.c +++ b/virtio/scsi.c @@ -174,7 +174,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) return size; } -static int get_vq_count(struct kvm *kvm, void *dev) +static u32 get_vq_count(struct kvm *kvm, void *dev) { return NUM_VIRT_QUEUES; } diff --git a/virtio/vsock.c b/virtio/vsock.c index 2df04d7..7d523df 100644 --- a/virtio/vsock.c +++ b/virtio/vsock.c @@ -202,7 +202,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi) die_perror("VHOST_SET_VRING_CALL failed"); } -static int get_vq_count(struct kvm *kvm, void *dev) +static u32 get_vq_count(struct kvm *kvm, void *dev) { return VSOCK_VQ_MAX; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL 2022-01-17 22:12 ` [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev @ 2022-02-01 14:57 ` Andre Przywara 2022-02-01 15:28 ` Alexandru Elisei 1 sibling, 0 replies; 20+ messages in thread From: Andre Przywara @ 2022-02-01 14:57 UTC (permalink / raw) To: Martin Radev Cc: kvm, will, julien.thierry.kdev, Jean-Philippe Brucker, Marc Zyngier, Alexandru Elisei On Tue, 18 Jan 2022 00:12:00 +0200 Martin Radev <martin.b.radev@gmail.com> wrote: Hi, > This patch checks for overflows in QUEUE_NOTIFY and QUEUE_SEL in > the PCI and MMIO operation handling paths. Further, the return > value type of get_vq_count is changed from int to u32 since negative > doesn't carry any semantic meaning. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > include/kvm/virtio.h | 2 +- > virtio/9p.c | 2 +- > virtio/balloon.c | 2 +- > virtio/blk.c | 2 +- > virtio/console.c | 2 +- > virtio/mmio.c | 25 ++++++++++++++++++++++--- > virtio/net.c | 2 +- > virtio/pci.c | 21 ++++++++++++++++++--- > virtio/rng.c | 2 +- > virtio/scsi.c | 2 +- > virtio/vsock.c | 2 +- > 11 files changed, 49 insertions(+), 15 deletions(-) > > diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h > index 3880e74..40f2a6d 100644 > --- a/include/kvm/virtio.h > +++ b/include/kvm/virtio.h > @@ -187,7 +187,7 @@ struct virtio_ops { > size_t (*get_config_size)(struct kvm *kvm, void *dev); > u32 (*get_host_features)(struct kvm *kvm, void *dev); > void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); > - int (*get_vq_count)(struct kvm *kvm, void *dev); > + u32 (*get_vq_count)(struct kvm *kvm, void *dev); I am not too happy about using an explicitly sized type for something that is not hardware related (should be just unsigned int), but it makes some sense below when we actually use the function. > int (*init_vq)(struct kvm *kvm, void *dev, u32 vq, u32 page_size, > u32 align, u32 pfn); > void (*exit_vq)(struct kvm *kvm, void *dev, u32 vq); > diff --git a/virtio/9p.c b/virtio/9p.c > index 89bec5e..8f1fc1f 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -1468,7 +1468,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/balloon.c b/virtio/balloon.c > index 233a3a5..de3882e 100644 > --- a/virtio/balloon.c > +++ b/virtio/balloon.c > @@ -249,7 +249,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/blk.c b/virtio/blk.c > index 9164b51..46918a4 100644 > --- a/virtio/blk.c > +++ b/virtio/blk.c > @@ -289,7 +289,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/console.c b/virtio/console.c > index 00bafa2..84466d0 100644 > --- a/virtio/console.c > +++ b/virtio/console.c > @@ -214,7 +214,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return VIRTIO_CONSOLE_NUM_QUEUES; > } > diff --git a/virtio/mmio.c b/virtio/mmio.c > index 32bba17..fd9a411 100644 > --- a/virtio/mmio.c > +++ b/virtio/mmio.c > @@ -171,14 +171,26 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, > struct virtio_mmio *vmmio = vdev->virtio; > struct kvm *kvm = vmmio->kvm; > u32 val = 0; > + u32 vq_count = 0; > + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); This should be on one line. > > switch (addr) { > case VIRTIO_MMIO_HOST_FEATURES_SEL: > case VIRTIO_MMIO_GUEST_FEATURES_SEL: > - case VIRTIO_MMIO_QUEUE_SEL: > val = ioport__read32(data); > *(u32 *)(((void *)&vmmio->hdr) + addr) = val; > break; > + case VIRTIO_MMIO_QUEUE_SEL: > + { Why the brackets here? > + val = ioport__read32(data); > + if (val >= vq_count) { > + pr_warning("QUEUE_SEL value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + break; > + } > + *(u32 *)(((void *)&vmmio->hdr) + addr) = val; > + break; > + } > case VIRTIO_MMIO_STATUS: > vmmio->hdr.status = ioport__read32(data); > if (!vmmio->hdr.status) /* Sample endianness on reset */ > @@ -222,6 +234,11 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, > break; > case VIRTIO_MMIO_QUEUE_NOTIFY: > val = ioport__read32(data); > + if (val > vq_count) { Shouldn't this be ">=" here? > + pr_warning("QUEUE_NOTIFY value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + break; > + } > vdev->ops->notify_vq(vmmio->kvm, vmmio->dev, val); > break; > case VIRTIO_MMIO_INTERRUPT_ACK: > @@ -341,10 +358,12 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev) > { > - int vq; > + u32 vq; > struct virtio_mmio *vmmio = vdev->virtio; > + u32 vq_count; Why this change (and the lines below)? Since get_vq_count() returns an u32 now, we should be safe? > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++) > + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); > + for (vq = 0; vq < vq_count; vq++) > virtio_mmio_exit_vq(kvm, vdev, vq); > > return 0; > diff --git a/virtio/net.c b/virtio/net.c > index 75d9ae5..9a25bfa 100644 > --- a/virtio/net.c > +++ b/virtio/net.c > @@ -753,7 +753,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > struct net_dev *ndev = dev; > > diff --git a/virtio/pci.c b/virtio/pci.c > index 50fdaa4..60ae2cb 100644 > --- a/virtio/pci.c > +++ b/virtio/pci.c > @@ -308,9 +308,11 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde > struct virtio_pci *vpci; > struct kvm *kvm; > u32 val; > + u32 vq_count; > > kvm = vcpu->kvm; > vpci = vdev->virtio; > + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); > > switch (offset) { > case VIRTIO_PCI_GUEST_FEATURES: > @@ -330,10 +332,21 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde > } > break; > case VIRTIO_PCI_QUEUE_SEL: > - vpci->queue_selector = ioport__read16(data); > + val = ioport__read16(data); > + if (val >= vq_count) { > + pr_warning("QUEUE_SEL value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + return false; > + } > + vpci->queue_selector = val; > break; > case VIRTIO_PCI_QUEUE_NOTIFY: > val = ioport__read16(data); > + if (val >= vq_count) { > + pr_warning("QUEUE_SEL value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + return false; > + } > vdev->ops->notify_vq(kvm, vpci->dev, val); > break; > case VIRTIO_PCI_STATUS: > @@ -626,10 +639,12 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev) > { > - int vq; > + u32 vq; > struct virtio_pci *vpci = vdev->virtio; > + u32 vq_count; Same here, looks like an unnecessary change, since vq_count is only used once below. Other than that it looks good in general. Cheers, Andre > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++) > + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); > + for (vq = 0; vq < vq_count; vq++) > virtio_pci_exit_vq(kvm, vdev, vq); > > return 0; > diff --git a/virtio/rng.c b/virtio/rng.c > index c7835a0..d9b9e68 100644 > --- a/virtio/rng.c > +++ b/virtio/rng.c > @@ -147,7 +147,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/scsi.c b/virtio/scsi.c > index 37418f8..cdf553d 100644 > --- a/virtio/scsi.c > +++ b/virtio/scsi.c > @@ -174,7 +174,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/vsock.c b/virtio/vsock.c > index 2df04d7..7d523df 100644 > --- a/virtio/vsock.c > +++ b/virtio/vsock.c > @@ -202,7 +202,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi) > die_perror("VHOST_SET_VRING_CALL failed"); > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return VSOCK_VQ_MAX; > } ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL 2022-01-17 22:12 ` [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev 2022-02-01 14:57 ` Andre Przywara @ 2022-02-01 15:28 ` Alexandru Elisei 1 sibling, 0 replies; 20+ messages in thread From: Alexandru Elisei @ 2022-02-01 15:28 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev Hi Martin, On Tue, Jan 18, 2022 at 12:12:00AM +0200, Martin Radev wrote: > This patch checks for overflows in QUEUE_NOTIFY and QUEUE_SEL in > the PCI and MMIO operation handling paths. Further, the return > value type of get_vq_count is changed from int to u32 since negative > doesn't carry any semantic meaning. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > include/kvm/virtio.h | 2 +- > virtio/9p.c | 2 +- > virtio/balloon.c | 2 +- > virtio/blk.c | 2 +- > virtio/console.c | 2 +- > virtio/mmio.c | 25 ++++++++++++++++++++++--- > virtio/net.c | 2 +- > virtio/pci.c | 21 ++++++++++++++++++--- > virtio/rng.c | 2 +- > virtio/scsi.c | 2 +- > virtio/vsock.c | 2 +- > 11 files changed, 49 insertions(+), 15 deletions(-) > > diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h > index 3880e74..40f2a6d 100644 > --- a/include/kvm/virtio.h > +++ b/include/kvm/virtio.h > @@ -187,7 +187,7 @@ struct virtio_ops { > size_t (*get_config_size)(struct kvm *kvm, void *dev); > u32 (*get_host_features)(struct kvm *kvm, void *dev); > void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); > - int (*get_vq_count)(struct kvm *kvm, void *dev); > + u32 (*get_vq_count)(struct kvm *kvm, void *dev); > int (*init_vq)(struct kvm *kvm, void *dev, u32 vq, u32 page_size, > u32 align, u32 pfn); > void (*exit_vq)(struct kvm *kvm, void *dev, u32 vq); > diff --git a/virtio/9p.c b/virtio/9p.c > index 89bec5e..8f1fc1f 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -1468,7 +1468,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/balloon.c b/virtio/balloon.c > index 233a3a5..de3882e 100644 > --- a/virtio/balloon.c > +++ b/virtio/balloon.c > @@ -249,7 +249,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/blk.c b/virtio/blk.c > index 9164b51..46918a4 100644 > --- a/virtio/blk.c > +++ b/virtio/blk.c > @@ -289,7 +289,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/console.c b/virtio/console.c > index 00bafa2..84466d0 100644 > --- a/virtio/console.c > +++ b/virtio/console.c > @@ -214,7 +214,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return VIRTIO_CONSOLE_NUM_QUEUES; > } > diff --git a/virtio/mmio.c b/virtio/mmio.c > index 32bba17..fd9a411 100644 > --- a/virtio/mmio.c > +++ b/virtio/mmio.c > @@ -171,14 +171,26 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, > struct virtio_mmio *vmmio = vdev->virtio; > struct kvm *kvm = vmmio->kvm; > u32 val = 0; > + u32 vq_count = 0; > + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); > > switch (addr) { > case VIRTIO_MMIO_HOST_FEATURES_SEL: > case VIRTIO_MMIO_GUEST_FEATURES_SEL: > - case VIRTIO_MMIO_QUEUE_SEL: > val = ioport__read32(data); > *(u32 *)(((void *)&vmmio->hdr) + addr) = val; > break; > + case VIRTIO_MMIO_QUEUE_SEL: > + { The braces aren't necessary here. > + val = ioport__read32(data); > + if (val >= vq_count) { > + pr_warning("QUEUE_SEL value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + break; > + } > + *(u32 *)(((void *)&vmmio->hdr) + addr) = val; > + break; > + } > case VIRTIO_MMIO_STATUS: > vmmio->hdr.status = ioport__read32(data); > if (!vmmio->hdr.status) /* Sample endianness on reset */ > @@ -222,6 +234,11 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu, > break; > case VIRTIO_MMIO_QUEUE_NOTIFY: > val = ioport__read32(data); > + if (val > vq_count) { "The driver notifies the device about new buffers being available in a queue by writing the index of the updated queue to QueueNotify". Shouldn't the inequality be val >= vq_count, as the maximum queue index is vq_count - 1? This is also how virtio-pci does the check for VIRTIO_PCI_QUEUE_NOTIFY. > + pr_warning("QUEUE_NOTIFY value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + break; > + } > vdev->ops->notify_vq(vmmio->kvm, vmmio->dev, val); > break; > case VIRTIO_MMIO_INTERRUPT_ACK: > @@ -341,10 +358,12 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > int virtio_mmio_reset(struct kvm *kvm, struct virtio_device *vdev) > { > - int vq; > + u32 vq; > struct virtio_mmio *vmmio = vdev->virtio; > + u32 vq_count; > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vmmio->dev); vq++) > + vq_count = vdev->ops->get_vq_count(kvm, vmmio->dev); > + for (vq = 0; vq < vq_count; vq++) Why this change? Thanks, Alex > virtio_mmio_exit_vq(kvm, vdev, vq); > > return 0; > diff --git a/virtio/net.c b/virtio/net.c > index 75d9ae5..9a25bfa 100644 > --- a/virtio/net.c > +++ b/virtio/net.c > @@ -753,7 +753,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > struct net_dev *ndev = dev; > > diff --git a/virtio/pci.c b/virtio/pci.c > index 50fdaa4..60ae2cb 100644 > --- a/virtio/pci.c > +++ b/virtio/pci.c > @@ -308,9 +308,11 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde > struct virtio_pci *vpci; > struct kvm *kvm; > u32 val; > + u32 vq_count; > > kvm = vcpu->kvm; > vpci = vdev->virtio; > + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); > > switch (offset) { > case VIRTIO_PCI_GUEST_FEATURES: > @@ -330,10 +332,21 @@ static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vde > } > break; > case VIRTIO_PCI_QUEUE_SEL: > - vpci->queue_selector = ioport__read16(data); > + val = ioport__read16(data); > + if (val >= vq_count) { > + pr_warning("QUEUE_SEL value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + return false; > + } > + vpci->queue_selector = val; > break; > case VIRTIO_PCI_QUEUE_NOTIFY: > val = ioport__read16(data); > + if (val >= vq_count) { > + pr_warning("QUEUE_SEL value (%u) is larger than VQ count (%u)\n", > + val, vq_count); > + return false; > + } > vdev->ops->notify_vq(kvm, vpci->dev, val); > break; > case VIRTIO_PCI_STATUS: > @@ -626,10 +639,12 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev, > > int virtio_pci__reset(struct kvm *kvm, struct virtio_device *vdev) > { > - int vq; > + u32 vq; > struct virtio_pci *vpci = vdev->virtio; > + u32 vq_count; > > - for (vq = 0; vq < vdev->ops->get_vq_count(kvm, vpci->dev); vq++) > + vq_count = vdev->ops->get_vq_count(kvm, vpci->dev); > + for (vq = 0; vq < vq_count; vq++) > virtio_pci_exit_vq(kvm, vdev, vq); > > return 0; > diff --git a/virtio/rng.c b/virtio/rng.c > index c7835a0..d9b9e68 100644 > --- a/virtio/rng.c > +++ b/virtio/rng.c > @@ -147,7 +147,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/scsi.c b/virtio/scsi.c > index 37418f8..cdf553d 100644 > --- a/virtio/scsi.c > +++ b/virtio/scsi.c > @@ -174,7 +174,7 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size) > return size; > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return NUM_VIRT_QUEUES; > } > diff --git a/virtio/vsock.c b/virtio/vsock.c > index 2df04d7..7d523df 100644 > --- a/virtio/vsock.c > +++ b/virtio/vsock.c > @@ -202,7 +202,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi) > die_perror("VHOST_SET_VRING_CALL failed"); > } > > -static int get_vq_count(struct kvm *kvm, void *dev) > +static u32 get_vq_count(struct kvm *kvm, void *dev) > { > return VSOCK_VQ_MAX; > } > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled 2022-01-17 22:11 [PATCH kvmtool 0/5] kvmtool: Fix few found bugs Martin Radev 2022-01-17 22:11 ` [PATCH kvmtool 1/5] virtio: Sanitize config accesses Martin Radev 2022-01-17 22:12 ` [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev @ 2022-01-17 22:12 ` Martin Radev 2022-02-01 14:57 ` Andre Przywara 2022-02-01 15:31 ` Alexandru Elisei 2022-01-17 22:12 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev 2022-01-17 22:12 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev 4 siblings, 2 replies; 20+ messages in thread From: Martin Radev @ 2022-01-17 22:12 UTC (permalink / raw) To: kvm; +Cc: will, julien.thierry.kdev, Martin Radev The virtio_net device is implicitly enabled if user doesn't explicitly specify that virtio_net is disabled. This is counter-intuitive to how the rest of the virtio commandline works. For backwards-compatibility, the commandline parameters are not changed. Instead, this patch prints out a warning if virtio_net is implicitly enabled. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- virtio/net.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/virtio/net.c b/virtio/net.c index 9a25bfa..ab75d40 100644 --- a/virtio/net.c +++ b/virtio/net.c @@ -1002,6 +1002,9 @@ int virtio_net__init(struct kvm *kvm) if (kvm->cfg.num_net_devices == 0 && kvm->cfg.no_net == 0) { static struct virtio_net_params net_params; + pr_warning( + "No net devices configured, but no_net not specified. " + "Enabling virtio_net with default network settings...\n"); net_params = (struct virtio_net_params) { .guest_ip = kvm->cfg.guest_ip, -- 2.25.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled 2022-01-17 22:12 ` [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled Martin Radev @ 2022-02-01 14:57 ` Andre Przywara 2022-02-01 15:31 ` Alexandru Elisei 1 sibling, 0 replies; 20+ messages in thread From: Andre Przywara @ 2022-02-01 14:57 UTC (permalink / raw) To: Martin Radev Cc: kvm, will, julien.thierry.kdev, Jean-Philippe Brucker, Marc Zyngier, Alexandru Elisei On Tue, 18 Jan 2022 00:12:01 +0200 Martin Radev <martin.b.radev@gmail.com> wrote: Hi, > The virtio_net device is implicitly enabled if user doesn't > explicitly specify that virtio_net is disabled. This is > counter-intuitive to how the rest of the virtio commandline > works. > > For backwards-compatibility, the commandline parameters are > not changed. Instead, this patch prints out a warning if > virtio_net is implicitly enabled. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > virtio/net.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/virtio/net.c b/virtio/net.c > index 9a25bfa..ab75d40 100644 > --- a/virtio/net.c > +++ b/virtio/net.c > @@ -1002,6 +1002,9 @@ int virtio_net__init(struct kvm *kvm) > > if (kvm->cfg.num_net_devices == 0 && kvm->cfg.no_net == 0) { > static struct virtio_net_params net_params; > + pr_warning( > + "No net devices configured, but no_net not specified. " > + "Enabling virtio_net with default network settings...\n"); I am a bit unsure about it. We recently tried to get rid of those molly guard messages, so I am not sure we should issue a *warning* here. After all, nothing is wrong, it's documented behaviour. Cheers, Andre > > net_params = (struct virtio_net_params) { > .guest_ip = kvm->cfg.guest_ip, ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled 2022-01-17 22:12 ` [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled Martin Radev 2022-02-01 14:57 ` Andre Przywara @ 2022-02-01 15:31 ` Alexandru Elisei 1 sibling, 0 replies; 20+ messages in thread From: Alexandru Elisei @ 2022-02-01 15:31 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev Hi Martin, On Tue, Jan 18, 2022 at 12:12:01AM +0200, Martin Radev wrote: > The virtio_net device is implicitly enabled if user doesn't > explicitly specify that virtio_net is disabled. This is > counter-intuitive to how the rest of the virtio commandline > works. > > For backwards-compatibility, the commandline parameters are > not changed. Instead, this patch prints out a warning if > virtio_net is implicitly enabled. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > virtio/net.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/virtio/net.c b/virtio/net.c > index 9a25bfa..ab75d40 100644 > --- a/virtio/net.c > +++ b/virtio/net.c > @@ -1002,6 +1002,9 @@ int virtio_net__init(struct kvm *kvm) > > if (kvm->cfg.num_net_devices == 0 && kvm->cfg.no_net == 0) { > static struct virtio_net_params net_params; > + pr_warning( > + "No net devices configured, but no_net not specified. " > + "Enabling virtio_net with default network settings...\n"); I don't think this deserves a warning, as there's nothing inherently wrong with having a default network device. In fact, this is quite helpful for users who want to get a VM up and running without complicated setup. I do agree that at the moment there's nothing to let the user know that a virtio network device is being created, not in the usage synopsis for the lkvm run command, nor in the man page for kvmtool, but I don't think that printing a warning is the solution here. What I suggest is that instead you print the virtual network interface parameters that can be useful for a user to know (like IP address and netmask, or whatever you think might be useful) using pr_debug() after virtio_net__init_one() is called successfully below. Thanks, Alex > > net_params = (struct virtio_net_params) { > .guest_ip = kvm->cfg.guest_ip, > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH kvmtool 4/5] Makefile: Mark stack as not executable 2022-01-17 22:11 [PATCH kvmtool 0/5] kvmtool: Fix few found bugs Martin Radev ` (2 preceding siblings ...) 2022-01-17 22:12 ` [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled Martin Radev @ 2022-01-17 22:12 ` Martin Radev 2022-02-01 15:01 ` Andre Przywara 2022-02-01 15:33 ` Alexandru Elisei 2022-01-17 22:12 ` [PATCH kvmtool 5/5] mmio: Sanitize addr and len Martin Radev 4 siblings, 2 replies; 20+ 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 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] 20+ messages in thread
* Re: [PATCH kvmtool 4/5] Makefile: Mark stack as not executable 2022-01-17 22:12 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev @ 2022-02-01 15:01 ` Andre Przywara 2022-02-01 15:33 ` Alexandru Elisei 1 sibling, 0 replies; 20+ messages in thread From: Andre Przywara @ 2022-02-01 15:01 UTC (permalink / raw) To: Martin Radev Cc: kvm, will, julien.thierry.kdev, Marc Zyngier, Alexandru Elisei, Jean-Philippe Brucker On Tue, 18 Jan 2022 00:12:02 +0200 Martin Radev <martin.b.radev@gmail.com> wrote: > This patch modifies CFLAGS to mark the stack explicitly > as not executable. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre > --- > Makefile | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index f251147..09ef282 100644 > --- a/Makefile > +++ b/Makefile > @@ -380,8 +380,11 @@ DEFINES += -D_GNU_SOURCE > DEFINES += -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"' > DEFINES += -DBUILD_ARCH='"$(ARCH)"' > > +# The stack doesn't need to be executable > +SECURITY_HARDENINGS := -z noexecstack > + > KVM_INCLUDE := include > -CFLAGS += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g > +CFLAGS += $(CPPFLAGS) $(DEFINES) $(SECURITY_HARDENINGS) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g > > WARNINGS += -Wall > WARNINGS += -Wformat=2 > @@ -582,4 +585,4 @@ ifneq ($(MAKECMDGOALS),clean) > > KVMTOOLS-VERSION-FILE: > @$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT) > -endif > \ No newline at end of file > +endif ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH kvmtool 4/5] Makefile: Mark stack as not executable 2022-01-17 22:12 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev 2022-02-01 15:01 ` Andre Przywara @ 2022-02-01 15:33 ` Alexandru Elisei 1 sibling, 0 replies; 20+ messages in thread From: Alexandru Elisei @ 2022-02-01 15:33 UTC (permalink / raw) To: Martin Radev; +Cc: kvm, will, julien.thierry.kdev Hi Martin, On Tue, Jan 18, 2022 at 12:12:02AM +0200, Martin Radev wrote: > This patch modifies CFLAGS to mark the stack explicitly > as not executable. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > Makefile | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index f251147..09ef282 100644 > --- a/Makefile > +++ b/Makefile > @@ -380,8 +380,11 @@ DEFINES += -D_GNU_SOURCE > DEFINES += -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"' > DEFINES += -DBUILD_ARCH='"$(ARCH)"' > > +# The stack doesn't need to be executable > +SECURITY_HARDENINGS := -z noexecstack > + > KVM_INCLUDE := include > -CFLAGS += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g > +CFLAGS += $(CPPFLAGS) $(DEFINES) $(SECURITY_HARDENINGS) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 -fno-strict-aliasing -g I used scanelf to check that the final binary has the stack marked as executable. For arm and arm64 I got this: $ scanelf -e lkvm TYPE STK/REL/PTL FILE ET_DYN RW- R-- RW- lkvm which as far as I can tell means the stack is not executable. For x86: $ scanelf -e lkvm TYPE STK/REL/PTL FILE ET_DYN RWX R-- RW- vm which means the stack is executable. Digging further, it looks like there are two objects which are missing the .note.GNU-stack section, x86/bios/entry.o and x86/bios/bios-rom.o. I suggest you try to fix the source files for those two objects before adding the flag to gcc. I used the Gentoo wiki [1] to diagnose the problem, in case it's useful to you. [1] https://wiki.gentoo.org/wiki/Hardened/GNU_stack_quickstart Thanks, Alex > > WARNINGS += -Wall > WARNINGS += -Wformat=2 > @@ -582,4 +585,4 @@ ifneq ($(MAKECMDGOALS),clean) > > KVMTOOLS-VERSION-FILE: > @$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT) > -endif > \ No newline at end of file > +endif > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 20+ 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 ` (3 preceding siblings ...) 2022-01-17 22:12 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev @ 2022-01-17 22:12 ` Martin Radev 2022-02-01 15:34 ` Alexandru Elisei 2022-02-01 15:52 ` Andre Przywara 4 siblings, 2 replies; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ messages in thread
* [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 5/5] mmio: Sanitize addr and len Martin Radev 0 siblings, 1 reply; 20+ 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] 20+ 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 @ 2022-03-03 23:10 ` Martin Radev 2022-03-16 15:39 ` Alexandru Elisei 0 siblings, 1 reply; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ messages in thread
end of thread, other threads:[~2022-04-22 10:37 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-17 22:11 [PATCH kvmtool 0/5] kvmtool: Fix few found bugs Martin Radev 2022-01-17 22:11 ` [PATCH kvmtool 1/5] virtio: Sanitize config accesses Martin Radev 2022-02-01 14:55 ` Andre Przywara 2022-02-01 15:27 ` Alexandru Elisei 2022-01-17 22:12 ` [PATCH kvmtool 2/5] virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL Martin Radev 2022-02-01 14:57 ` Andre Przywara 2022-02-01 15:28 ` Alexandru Elisei 2022-01-17 22:12 ` [PATCH kvmtool 3/5] virtio/net: Warn if virtio_net is implicitly enabled Martin Radev 2022-02-01 14:57 ` Andre Przywara 2022-02-01 15:31 ` Alexandru Elisei 2022-01-17 22:12 ` [PATCH kvmtool 4/5] Makefile: Mark stack as not executable Martin Radev 2022-02-01 15:01 ` Andre Przywara 2022-02-01 15:33 ` Alexandru Elisei 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 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 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
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.