| Message ID | 20220303231050.2146621-3-martin.b.radev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix few small issues in virtio code | expand |
Hi, On Fri, Mar 04, 2022 at 01:10:47AM +0200, Martin Radev wrote: > The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows. > This patch sanitizes this operation by using the newly added virtio op > get_config_size. Any access which goes beyond the config structure's > size is prevented and a failure is returned. > > Additionally, PCI accesses which span more than a single byte are prevented > and a warning is printed because the implementation does not currently > support the behavior correctly. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > include/kvm/virtio-9p.h | 1 + > include/kvm/virtio.h | 1 + > virtio/9p.c | 25 ++++++++++++++++++++----- > virtio/balloon.c | 8 ++++++++ > virtio/blk.c | 8 ++++++++ > virtio/console.c | 8 ++++++++ > virtio/mmio.c | 24 ++++++++++++++++++++---- > virtio/net.c | 8 ++++++++ > virtio/pci.c | 38 ++++++++++++++++++++++++++++++++++++++ > virtio/rng.c | 6 ++++++ > virtio/scsi.c | 8 ++++++++ > virtio/vsock.c | 8 ++++++++ > 12 files changed, 134 insertions(+), 9 deletions(-) > > diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h > index 3ea7698..77c5062 100644 > --- a/include/kvm/virtio-9p.h > +++ b/include/kvm/virtio-9p.h > @@ -44,6 +44,7 @@ struct p9_dev { > struct virtio_device vdev; > struct rb_root fids; > > + size_t config_size; > struct virtio_9p_config *config; > u32 features; > > diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h > index 3a311f5..3880e74 100644 > --- a/include/kvm/virtio.h > +++ b/include/kvm/virtio.h > @@ -184,6 +184,7 @@ struct virtio_device { > > struct virtio_ops { > u8 *(*get_config)(struct kvm *kvm, void *dev); > + size_t (*get_config_size)(struct kvm *kvm, void *dev); > u32 (*get_host_features)(struct kvm *kvm, void *dev); > void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); > int (*get_vq_count)(struct kvm *kvm, void *dev); > diff --git a/virtio/9p.c b/virtio/9p.c > index b78f2b3..6074f3a 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -1375,6 +1375,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(p9dev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + struct p9_dev *p9dev = dev; > + > + return p9dev->config_size; > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1 << VIRTIO_9P_MOUNT_TAG; > @@ -1469,6 +1476,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > struct virtio_ops p9_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > @@ -1568,7 +1576,9 @@ virtio_dev_init(virtio_9p__init); > int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) > { > struct p9_dev *p9dev; > - int err = 0; > + size_t tag_name_length; I think it would be better to name the variable tag_len, the same name as the corresponding field in struct virtio_9p_config. As a bonus, it's also shorter. But this is personal preference in the end, so I leave it up to you to decide which works better. > + size_t config_size; > + int err; > > p9dev = calloc(1, sizeof(*p9dev)); > if (!p9dev) > @@ -1577,29 +1587,34 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) > if (!tag_name) > tag_name = VIRTIO_9P_DEFAULT_TAG; > > - p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1); > + tag_name_length = strlen(tag_name); > + /* The tag_name zero byte is intentionally excluded */ If this is indeed a bug (the comment from virtio_9p_config seems to suggest it is, but I couldn't find the 9p spec), the bug is that the config size is computed incorrectly, which is a different bug than a guest being able to write outside of the config region for the device. As such, it should be fixed in a separate patch. > + config_size = sizeof(*p9dev->config) + tag_name_length; > + > + p9dev->config = calloc(1, config_size); > if (p9dev->config == NULL) { > err = -ENOMEM; > goto free_p9dev; > } > + p9dev->config_size = config_size; > > strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir)); > p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00'; > > - p9dev->config->tag_len = strlen(tag_name); > + p9dev->config->tag_len = tag_name_length; > if (p9dev->config->tag_len > MAX_TAG_LEN) { > err = -EINVAL; > goto free_p9dev_config; > } > > - memcpy(&p9dev->config->tag, tag_name, strlen(tag_name)); > + memcpy(&p9dev->config->tag, tag_name, tag_name_length); > > list_add(&p9dev->list, &devs); > > if (compat_id == -1) > compat_id = virtio_compat_add_message("virtio-9p", "CONFIG_NET_9P_VIRTIO"); > > - return err; > + return 0; > > free_p9dev_config: > free(p9dev->config); > diff --git a/virtio/balloon.c b/virtio/balloon.c > index 8e8803f..5bcd6ab 100644 > --- a/virtio/balloon.c > +++ b/virtio/balloon.c > @@ -181,6 +181,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&bdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + struct bln_dev *bdev = dev; > + > + return sizeof(bdev->config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1 << VIRTIO_BALLOON_F_STATS_VQ; > @@ -251,6 +258,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > struct virtio_ops bln_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > diff --git a/virtio/blk.c b/virtio/blk.c > index 4d02d10..af71c0c 100644 > --- a/virtio/blk.c > +++ b/virtio/blk.c > @@ -146,6 +146,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&bdev->blk_config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + struct blk_dev *bdev = dev; > + > + return sizeof(bdev->blk_config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > struct blk_dev *bdev = dev; > @@ -291,6 +298,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops blk_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .get_vq_count = get_vq_count, > diff --git a/virtio/console.c b/virtio/console.c > index e0b98df..dae6034 100644 > --- a/virtio/console.c > +++ b/virtio/console.c > @@ -121,6 +121,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&cdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + struct con_dev *cdev = dev; > + > + return sizeof(cdev->config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 0; > @@ -216,6 +223,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops con_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .get_vq_count = get_vq_count, > diff --git a/virtio/mmio.c b/virtio/mmio.c > index 875a288..0094856 100644 > --- a/virtio/mmio.c > +++ b/virtio/mmio.c > @@ -103,15 +103,31 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu, > u8 is_write, struct virtio_device *vdev) > { > struct virtio_mmio *vmmio = vdev->virtio; > + u8 *config_aperture; > + size_t config_aperture_size; These could be shortened to config and config_size, to better match the callback names (get_config, respectively get_config_size) and make the code easier to understand. > u32 i; > > + /* Check for wrap-around. */ > + if (addr + len < addr) { No need for this, you can move patch #5 before this one, and that should take care of any wrap arounds. > + WARN_ONCE(1, "addr (%llu) + length (%u) wraps-around.\n", addr, len); > + return; > + } > + > + config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev); > + config_aperture_size = vdev->ops->get_config_size(vmmio->kvm, vmmio->dev); > + > + /* Prevent invalid accesses which go beyond the config */ > + if (config_aperture_size < addr + len) { > + WARN_ONCE(1, "Offset (%llu) Length (%u) goes beyond config size (%zu).\n", > + addr, len, config_aperture_size); > + return; > + } > + > for (i = 0; i < len; i++) { > if (is_write) > - vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] = > - *(u8 *)data + i; > + config_aperture[addr + i] = *(u8 *)data + i; > else > - data[i] = vdev->ops->get_config(vmmio->kvm, > - vmmio->dev)[addr + i]; > + data[i] = config_aperture[addr + i]; > } > } > > diff --git a/virtio/net.c b/virtio/net.c > index 1ee3c19..ec5dc1f 100644 > --- a/virtio/net.c > +++ b/virtio/net.c > @@ -480,6 +480,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&ndev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + struct net_dev *ndev = dev; > + > + return sizeof(ndev->config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > u32 features; > @@ -757,6 +764,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops net_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .get_vq_count = get_vq_count, > diff --git a/virtio/pci.c b/virtio/pci.c > index 2777d1c..0b5cccd 100644 > --- a/virtio/pci.c > +++ b/virtio/pci.c > @@ -136,7 +136,25 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device * > return true; > } else if (type == VIRTIO_PCI_O_CONFIG) { > u8 cfg; > + size_t config_size; > > + config_size = vdev->ops->get_config_size(kvm, vpci->dev); > + if (size <= 0) { > + WARN_ONCE(1, "Size (%d) is non-positive\n", size); > + return false; > + } This is a different bug. The kvm_run.mmio struct report length as an u32 and the type is preserved until virtio_pci__data_{in,out} are called from virtio_pci__mmio_callback(). The correct fix is to change the size parameter from virtio_pci__data_{in,out} and virtio_pci__specific_data_{in,out} to an u32 in a separate patch. [1] https://elixir.bootlin.com/linux/v5.17-rc8/source/Documentation/virt/kvm/api.rst#L5726 Thanks, Alex > + if (config_offset + (u32)size > config_size) { > + /* Access goes beyond the config size, so return failure. */ > + WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n", > + config_offset, config_size); > + return false; > + } > + > + /* TODO: Handle access lengths beyond one byte */ > + if (size != 1) { > + WARN_ONCE(1, "Size (%d) not supported\n", size); > + return false; > + } > cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset]; > ioport__write8(data, cfg); > return true; > @@ -276,6 +294,26 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device > > return true; > } else if (type == VIRTIO_PCI_O_CONFIG) { > + size_t config_size; > + > + if (size <= 0) { > + WARN_ONCE(1, "Size (%d) is non-positive\n", size); > + return false; > + } > + > + config_size = vdev->ops->get_config_size(kvm, vpci->dev); > + if (config_offset + (u32)size > config_size) { > + /* Access goes beyond the config size, so return failure. */ > + WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n", > + config_offset, config_size); > + return false; > + } > + > + /* TODO: Handle access lengths beyond one byte */ > + if (size != 1) { > + WARN_ONCE(1, "Size (%d) not supported\n", size); > + return false; > + } > vdev->ops->get_config(kvm, vpci->dev)[config_offset] = *(u8 *)data; > > return true; > diff --git a/virtio/rng.c b/virtio/rng.c > index 78eaa64..c7835a0 100644 > --- a/virtio/rng.c > +++ b/virtio/rng.c > @@ -47,6 +47,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return 0; > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + return 0; > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > /* Unused */ > @@ -149,6 +154,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops rng_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > diff --git a/virtio/scsi.c b/virtio/scsi.c > index 16a86cb..8f1c348 100644 > --- a/virtio/scsi.c > +++ b/virtio/scsi.c > @@ -38,6 +38,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&sdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + struct scsi_dev *sdev = dev; > + > + return sizeof(sdev->config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1UL << VIRTIO_RING_F_EVENT_IDX | > @@ -176,6 +183,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops scsi_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > diff --git a/virtio/vsock.c b/virtio/vsock.c > index 5b99838..34397b6 100644 > --- a/virtio/vsock.c > +++ b/virtio/vsock.c > @@ -41,6 +41,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) > return ((u8 *)(&vdev->config)); > } > > +static size_t get_config_size(struct kvm *kvm, void *dev) > +{ > + struct vsock_dev *vdev = dev; > + > + return sizeof(vdev->config); > +} > + > static u32 get_host_features(struct kvm *kvm, void *dev) > { > return 1UL << VIRTIO_RING_F_EVENT_IDX > @@ -204,6 +211,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > static struct virtio_ops vsock_dev_virtio_ops = { > .get_config = get_config, > + .get_config_size = get_config_size, > .get_host_features = get_host_features, > .set_guest_features = set_guest_features, > .init_vq = init_vq, > -- > 2.25.1 >
Thank you for the review. Answers are inline. Here are the two patches: int to u32 patch: From ddedd3a59b41d97e07deac59af177b360cc04b20 Mon Sep 17 00:00:00 2001 From: Martin Radev <martin.b.radev@gmail.com> Date: Thu, 24 Mar 2022 23:24:57 +0200 Subject: [PATCH kvmtool 3/6] virtio: Use u32 instead of int in pci_data_in/out The PCI access size type is changed from a signed type to an unsigned type since the size is never expected to be negative, and the type also matches the type in the signature of virtio_pci__io_mmio_callback. This change simplifies size checking in the next patch. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- virtio/pci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/virtio/pci.c b/virtio/pci.c index 2777d1c..bcb205a 100644 --- a/virtio/pci.c +++ b/virtio/pci.c @@ -116,7 +116,7 @@ static inline bool virtio_pci__msix_enabled(struct virtio_pci *vpci) } static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device *vdev, - void *data, int size, unsigned long offset) + void *data, u32 size, unsigned long offset) { u32 config_offset; struct virtio_pci *vpci = vdev->virtio; @@ -146,7 +146,7 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device * } static bool virtio_pci__data_in(struct kvm_cpu *vcpu, struct virtio_device *vdev, - unsigned long offset, void *data, int size) + unsigned long offset, void *data, u32 size) { bool ret = true; struct virtio_pci *vpci; @@ -211,7 +211,7 @@ static void update_msix_map(struct virtio_pci *vpci, } static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device *vdev, - void *data, int size, unsigned long offset) + void *data, u32 size, unsigned long offset) { struct virtio_pci *vpci = vdev->virtio; u32 config_offset, vec; @@ -285,7 +285,7 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device } static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vdev, - unsigned long offset, void *data, int size) + unsigned long offset, void *data, u32 size) { bool ret = true; struct virtio_pci *vpci;
Hi, On Sun, Mar 27, 2022 at 11:37:00PM +0300, Martin Radev wrote: > > Thank you for the review. > Answers are inline. > Here are the two patches: > > int to u32 patch: > > From ddedd3a59b41d97e07deac59af177b360cc04b20 Mon Sep 17 00:00:00 2001 > From: Martin Radev <martin.b.radev@gmail.com> > Date: Thu, 24 Mar 2022 23:24:57 +0200 > Subject: [PATCH kvmtool 3/6] virtio: Use u32 instead of int in pci_data_in/out > > The PCI access size type is changed from a signed type > to an unsigned type since the size is never expected to > be negative, and the type also matches the type in the > signature of virtio_pci__io_mmio_callback. > This change simplifies size checking in the next patch. > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > --- > virtio/pci.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/virtio/pci.c b/virtio/pci.c > index 2777d1c..bcb205a 100644 > --- a/virtio/pci.c > +++ b/virtio/pci.c > @@ -116,7 +116,7 @@ static inline bool virtio_pci__msix_enabled(struct virtio_pci *vpci) > } > > static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device *vdev, > - void *data, int size, unsigned long offset) > + void *data, u32 size, unsigned long offset) > { > u32 config_offset; > struct virtio_pci *vpci = vdev->virtio; > @@ -146,7 +146,7 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device * > } > > static bool virtio_pci__data_in(struct kvm_cpu *vcpu, struct virtio_device *vdev, > - unsigned long offset, void *data, int size) > + unsigned long offset, void *data, u32 size) > { > bool ret = true; > struct virtio_pci *vpci; > @@ -211,7 +211,7 @@ static void update_msix_map(struct virtio_pci *vpci, > } > > static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device *vdev, > - void *data, int size, unsigned long offset) > + void *data, u32 size, unsigned long offset) > { > struct virtio_pci *vpci = vdev->virtio; > u32 config_offset, vec; > @@ -285,7 +285,7 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device > } > > static bool virtio_pci__data_out(struct kvm_cpu *vcpu, struct virtio_device *vdev, > - unsigned long offset, void *data, int size) > + unsigned long offset, void *data, u32 size) > { > bool ret = true; > struct virtio_pci *vpci; > -- > 2.25.1 That looks good to me. Please send it as a separate patch in the next version of the series. > > Original patch but with comments addressed: If you change the patch, the new version should be sent as a new series. The version of the series should be reflected in the subject of each patch. For example, this series should have been v2, which means the prefix for the patches should have been: PATCH v2 kvmtool [..]. This can be accomplished with git format-patch directly, by using the command line argument --subject-prefix="PATCH v2 kvmtool". The next iteration of the series will be v3, and so on. This is done to make it easier for everyone to keep track of the latest version of a patch set. It's also easier to review and test a new version of a patch when it is standalone than when it is attached to an email containing several other things. For example, you've attached two patches to this email, which can cause confusion about the order of the patches. This can be easily avoided by sending the changes in a separate series. I'll reply to your comments below. > On Wed, Mar 16, 2022 at 01:04:08PM +0000, Alexandru Elisei wrote: > > Hi, > > > > On Fri, Mar 04, 2022 at 01:10:47AM +0200, Martin Radev wrote: > > > The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows. > > > This patch sanitizes this operation by using the newly added virtio op > > > get_config_size. Any access which goes beyond the config structure's > > > size is prevented and a failure is returned. > > > > > > Additionally, PCI accesses which span more than a single byte are prevented > > > and a warning is printed because the implementation does not currently > > > support the behavior correctly. > > > > > > Signed-off-by: Martin Radev <martin.b.radev@gmail.com> > > > --- > > > include/kvm/virtio-9p.h | 1 + > > > include/kvm/virtio.h | 1 + > > > virtio/9p.c | 25 ++++++++++++++++++++----- > > > virtio/balloon.c | 8 ++++++++ > > > virtio/blk.c | 8 ++++++++ > > > virtio/console.c | 8 ++++++++ > > > virtio/mmio.c | 24 ++++++++++++++++++++---- > > > virtio/net.c | 8 ++++++++ > > > virtio/pci.c | 38 ++++++++++++++++++++++++++++++++++++++ > > > virtio/rng.c | 6 ++++++ > > > virtio/scsi.c | 8 ++++++++ > > > virtio/vsock.c | 8 ++++++++ > > > 12 files changed, 134 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h > > > index 3ea7698..77c5062 100644 > > > --- a/include/kvm/virtio-9p.h > > > +++ b/include/kvm/virtio-9p.h > > > @@ -44,6 +44,7 @@ struct p9_dev { > > > struct virtio_device vdev; > > > struct rb_root fids; > > > > > > + size_t config_size; > > > struct virtio_9p_config *config; > > > u32 features; > > > > > > diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h > > > index 3a311f5..3880e74 100644 > > > --- a/include/kvm/virtio.h > > > +++ b/include/kvm/virtio.h > > > @@ -184,6 +184,7 @@ struct virtio_device { > > > > > > struct virtio_ops { > > > u8 *(*get_config)(struct kvm *kvm, void *dev); > > > + size_t (*get_config_size)(struct kvm *kvm, void *dev); > > > u32 (*get_host_features)(struct kvm *kvm, void *dev); > > > void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); > > > int (*get_vq_count)(struct kvm *kvm, void *dev); > > > diff --git a/virtio/9p.c b/virtio/9p.c > > > index b78f2b3..6074f3a 100644 > > > --- a/virtio/9p.c > > > +++ b/virtio/9p.c > > > @@ -1375,6 +1375,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) > > > return ((u8 *)(p9dev->config)); > > > } > > > > > > +static size_t get_config_size(struct kvm *kvm, void *dev) > > > +{ > > > + struct p9_dev *p9dev = dev; > > > + > > > + return p9dev->config_size; > > > +} > > > + > > > static u32 get_host_features(struct kvm *kvm, void *dev) > > > { > > > return 1 << VIRTIO_9P_MOUNT_TAG; > > > @@ -1469,6 +1476,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) > > > > > > struct virtio_ops p9_dev_virtio_ops = { > > > .get_config = get_config, > > > + .get_config_size = get_config_size, > > > .get_host_features = get_host_features, > > > .set_guest_features = set_guest_features, > > > .init_vq = init_vq, > > > @@ -1568,7 +1576,9 @@ virtio_dev_init(virtio_9p__init); > > > int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) > > > { > > > struct p9_dev *p9dev; > > > - int err = 0; > > > + size_t tag_name_length; > > > > I think it would be better to name the variable tag_len, the same name as > > the corresponding field in struct virtio_9p_config. As a bonus, it's also > > shorter. But this is personal preference in the end, so I leave it up to > > you to decide which works better. > > > Done. > > > > + size_t config_size; > > > + int err; > > > > > > p9dev = calloc(1, sizeof(*p9dev)); > > > if (!p9dev) > > > @@ -1577,29 +1587,34 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) > > > if (!tag_name) > > > tag_name = VIRTIO_9P_DEFAULT_TAG; > > > > > > - p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1); > > > + tag_name_length = strlen(tag_name); > > > + /* The tag_name zero byte is intentionally excluded */ > > > > If this is indeed a bug (the comment from virtio_9p_config seems to suggest > > it is, but I couldn't find the 9p spec), the bug is that the config size is > > computed incorrectly, which is a different bug than a guest being able to > > write outside of the config region for the device. As such, it should be > > fixed in a separate patch. > > > I couldn't find information about how large the configuration size is and > whether the 0 byte is included. QEMU explicitly excludes it. > See https://elixir.bootlin.com/qemu/latest/source/hw/9pfs/virtio-9p-device.c#L218 > I think this is almost surely the correct way considering the tag length > is also part of the config. I think I haven't managed to make myself clear. I agree that the NUL terminating byte shouldn't be taken into account when calculating the config size. What I was referring to is the fact that there two different bugs here: 1. The config size is calculated incorrectly in the existing code: p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1); The code shouldn't add that one byte to the size, which presumably represent the NUL terminating byte from the tag_name string. 2. The code doesn't check for overflow. Your patch tries to fix both bugs in one go. What I was suggesting is to write a standalone patch that fixes bug #1, and keep this patch that adds the overflow check, minus the fix for #1. This makes everything cleaner, easier to review and test, and easier to diagnose and fix or revert if the fix turns out to be wrong. Thanks, Alex
diff --git a/include/kvm/virtio-9p.h b/include/kvm/virtio-9p.h index 3ea7698..77c5062 100644 --- a/include/kvm/virtio-9p.h +++ b/include/kvm/virtio-9p.h @@ -44,6 +44,7 @@ struct p9_dev { struct virtio_device vdev; struct rb_root fids; + size_t config_size; struct virtio_9p_config *config; u32 features; diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h index 3a311f5..3880e74 100644 --- a/include/kvm/virtio.h +++ b/include/kvm/virtio.h @@ -184,6 +184,7 @@ struct virtio_device { struct virtio_ops { u8 *(*get_config)(struct kvm *kvm, void *dev); + size_t (*get_config_size)(struct kvm *kvm, void *dev); u32 (*get_host_features)(struct kvm *kvm, void *dev); void (*set_guest_features)(struct kvm *kvm, void *dev, u32 features); int (*get_vq_count)(struct kvm *kvm, void *dev); diff --git a/virtio/9p.c b/virtio/9p.c index b78f2b3..6074f3a 100644 --- a/virtio/9p.c +++ b/virtio/9p.c @@ -1375,6 +1375,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(p9dev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + struct p9_dev *p9dev = dev; + + return p9dev->config_size; +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 1 << VIRTIO_9P_MOUNT_TAG; @@ -1469,6 +1476,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) struct virtio_ops p9_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq, @@ -1568,7 +1576,9 @@ virtio_dev_init(virtio_9p__init); int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) { struct p9_dev *p9dev; - int err = 0; + size_t tag_name_length; + size_t config_size; + int err; p9dev = calloc(1, sizeof(*p9dev)); if (!p9dev) @@ -1577,29 +1587,34 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name) if (!tag_name) tag_name = VIRTIO_9P_DEFAULT_TAG; - p9dev->config = calloc(1, sizeof(*p9dev->config) + strlen(tag_name) + 1); + tag_name_length = strlen(tag_name); + /* The tag_name zero byte is intentionally excluded */ + config_size = sizeof(*p9dev->config) + tag_name_length; + + p9dev->config = calloc(1, config_size); if (p9dev->config == NULL) { err = -ENOMEM; goto free_p9dev; } + p9dev->config_size = config_size; strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir)); p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00'; - p9dev->config->tag_len = strlen(tag_name); + p9dev->config->tag_len = tag_name_length; if (p9dev->config->tag_len > MAX_TAG_LEN) { err = -EINVAL; goto free_p9dev_config; } - memcpy(&p9dev->config->tag, tag_name, strlen(tag_name)); + memcpy(&p9dev->config->tag, tag_name, tag_name_length); list_add(&p9dev->list, &devs); if (compat_id == -1) compat_id = virtio_compat_add_message("virtio-9p", "CONFIG_NET_9P_VIRTIO"); - return err; + return 0; free_p9dev_config: free(p9dev->config); diff --git a/virtio/balloon.c b/virtio/balloon.c index 8e8803f..5bcd6ab 100644 --- a/virtio/balloon.c +++ b/virtio/balloon.c @@ -181,6 +181,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&bdev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + struct bln_dev *bdev = dev; + + return sizeof(bdev->config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 1 << VIRTIO_BALLOON_F_STATS_VQ; @@ -251,6 +258,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) struct virtio_ops bln_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq, diff --git a/virtio/blk.c b/virtio/blk.c index 4d02d10..af71c0c 100644 --- a/virtio/blk.c +++ b/virtio/blk.c @@ -146,6 +146,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&bdev->blk_config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + struct blk_dev *bdev = dev; + + return sizeof(bdev->blk_config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { struct blk_dev *bdev = dev; @@ -291,6 +298,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops blk_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .get_vq_count = get_vq_count, diff --git a/virtio/console.c b/virtio/console.c index e0b98df..dae6034 100644 --- a/virtio/console.c +++ b/virtio/console.c @@ -121,6 +121,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&cdev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + struct con_dev *cdev = dev; + + return sizeof(cdev->config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 0; @@ -216,6 +223,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops con_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .get_vq_count = get_vq_count, diff --git a/virtio/mmio.c b/virtio/mmio.c index 875a288..0094856 100644 --- a/virtio/mmio.c +++ b/virtio/mmio.c @@ -103,15 +103,31 @@ static void virtio_mmio_device_specific(struct kvm_cpu *vcpu, u8 is_write, struct virtio_device *vdev) { struct virtio_mmio *vmmio = vdev->virtio; + u8 *config_aperture; + size_t config_aperture_size; u32 i; + /* Check for wrap-around. */ + if (addr + len < addr) { + WARN_ONCE(1, "addr (%llu) + length (%u) wraps-around.\n", addr, len); + return; + } + + config_aperture = vdev->ops->get_config(vmmio->kvm, vmmio->dev); + config_aperture_size = vdev->ops->get_config_size(vmmio->kvm, vmmio->dev); + + /* Prevent invalid accesses which go beyond the config */ + if (config_aperture_size < addr + len) { + WARN_ONCE(1, "Offset (%llu) Length (%u) goes beyond config size (%zu).\n", + addr, len, config_aperture_size); + return; + } + for (i = 0; i < len; i++) { if (is_write) - vdev->ops->get_config(vmmio->kvm, vmmio->dev)[addr + i] = - *(u8 *)data + i; + config_aperture[addr + i] = *(u8 *)data + i; else - data[i] = vdev->ops->get_config(vmmio->kvm, - vmmio->dev)[addr + i]; + data[i] = config_aperture[addr + i]; } } diff --git a/virtio/net.c b/virtio/net.c index 1ee3c19..ec5dc1f 100644 --- a/virtio/net.c +++ b/virtio/net.c @@ -480,6 +480,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&ndev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + struct net_dev *ndev = dev; + + return sizeof(ndev->config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { u32 features; @@ -757,6 +764,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops net_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .get_vq_count = get_vq_count, diff --git a/virtio/pci.c b/virtio/pci.c index 2777d1c..0b5cccd 100644 --- a/virtio/pci.c +++ b/virtio/pci.c @@ -136,7 +136,25 @@ static bool virtio_pci__specific_data_in(struct kvm *kvm, struct virtio_device * return true; } else if (type == VIRTIO_PCI_O_CONFIG) { u8 cfg; + size_t config_size; + config_size = vdev->ops->get_config_size(kvm, vpci->dev); + if (size <= 0) { + WARN_ONCE(1, "Size (%d) is non-positive\n", size); + return false; + } + if (config_offset + (u32)size > config_size) { + /* Access goes beyond the config size, so return failure. */ + WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n", + config_offset, config_size); + return false; + } + + /* TODO: Handle access lengths beyond one byte */ + if (size != 1) { + WARN_ONCE(1, "Size (%d) not supported\n", size); + return false; + } cfg = vdev->ops->get_config(kvm, vpci->dev)[config_offset]; ioport__write8(data, cfg); return true; @@ -276,6 +294,26 @@ static bool virtio_pci__specific_data_out(struct kvm *kvm, struct virtio_device return true; } else if (type == VIRTIO_PCI_O_CONFIG) { + size_t config_size; + + if (size <= 0) { + WARN_ONCE(1, "Size (%d) is non-positive\n", size); + return false; + } + + config_size = vdev->ops->get_config_size(kvm, vpci->dev); + if (config_offset + (u32)size > config_size) { + /* Access goes beyond the config size, so return failure. */ + WARN_ONCE(1, "Config access offset (%u) is beyond config size (%zu)\n", + config_offset, config_size); + return false; + } + + /* TODO: Handle access lengths beyond one byte */ + if (size != 1) { + WARN_ONCE(1, "Size (%d) not supported\n", size); + return false; + } vdev->ops->get_config(kvm, vpci->dev)[config_offset] = *(u8 *)data; return true; diff --git a/virtio/rng.c b/virtio/rng.c index 78eaa64..c7835a0 100644 --- a/virtio/rng.c +++ b/virtio/rng.c @@ -47,6 +47,11 @@ static u8 *get_config(struct kvm *kvm, void *dev) return 0; } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + return 0; +} + static u32 get_host_features(struct kvm *kvm, void *dev) { /* Unused */ @@ -149,6 +154,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops rng_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq, diff --git a/virtio/scsi.c b/virtio/scsi.c index 16a86cb..8f1c348 100644 --- a/virtio/scsi.c +++ b/virtio/scsi.c @@ -38,6 +38,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&sdev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + struct scsi_dev *sdev = dev; + + return sizeof(sdev->config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 1UL << VIRTIO_RING_F_EVENT_IDX | @@ -176,6 +183,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops scsi_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq, diff --git a/virtio/vsock.c b/virtio/vsock.c index 5b99838..34397b6 100644 --- a/virtio/vsock.c +++ b/virtio/vsock.c @@ -41,6 +41,13 @@ static u8 *get_config(struct kvm *kvm, void *dev) return ((u8 *)(&vdev->config)); } +static size_t get_config_size(struct kvm *kvm, void *dev) +{ + struct vsock_dev *vdev = dev; + + return sizeof(vdev->config); +} + static u32 get_host_features(struct kvm *kvm, void *dev) { return 1UL << VIRTIO_RING_F_EVENT_IDX @@ -204,6 +211,7 @@ static int get_vq_count(struct kvm *kvm, void *dev) static struct virtio_ops vsock_dev_virtio_ops = { .get_config = get_config, + .get_config_size = get_config_size, .get_host_features = get_host_features, .set_guest_features = set_guest_features, .init_vq = init_vq,
The handling of VIRTIO_PCI_O_CONFIG is prone to buffer access overflows. This patch sanitizes this operation by using the newly added virtio op get_config_size. Any access which goes beyond the config structure's size is prevented and a failure is returned. Additionally, PCI accesses which span more than a single byte are prevented and a warning is printed because the implementation does not currently support the behavior correctly. Signed-off-by: Martin Radev <martin.b.radev@gmail.com> --- include/kvm/virtio-9p.h | 1 + include/kvm/virtio.h | 1 + virtio/9p.c | 25 ++++++++++++++++++++----- virtio/balloon.c | 8 ++++++++ virtio/blk.c | 8 ++++++++ virtio/console.c | 8 ++++++++ virtio/mmio.c | 24 ++++++++++++++++++++---- virtio/net.c | 8 ++++++++ virtio/pci.c | 38 ++++++++++++++++++++++++++++++++++++++ virtio/rng.c | 6 ++++++ virtio/scsi.c | 8 ++++++++ virtio/vsock.c | 8 ++++++++ 12 files changed, 134 insertions(+), 9 deletions(-)