Hi All: This series treis to do more hardening for virito. patch 1 validates the num_queues for virio-blk device. patch 2-4 validates max_nr_ports for virito-console device. patch 5-7 harden virtio-pci interrupts to make sure no exepcted interrupt handler is tiggered. If this makes sense we can do similar things in other transport drivers. patch 8-9 validate used ring length. Smoking test on blk/net with packed=on/off and iommu_platform=on/off. Please review. Thanks Jason Wang (9): virtio-blk: validate num_queues during probe virtio: add doc for validate() method virtio-console: switch to use .validate() virtio_console: validate max_nr_ports before trying to use it virtio_config: introduce a new ready method virtio_pci: harden MSI-X interrupts virtio-pci: harden INTX interrupts virtio_ring: fix typos in vring_desc_extra virtio_ring: validate used buffer length drivers/block/virtio_blk.c | 3 +- drivers/char/virtio_console.c | 51 +++++++++++++++++++++--------- drivers/virtio/virtio_pci_common.c | 43 +++++++++++++++++++++---- drivers/virtio/virtio_pci_common.h | 7 ++-- drivers/virtio/virtio_pci_legacy.c | 5 +-- drivers/virtio/virtio_pci_modern.c | 6 ++-- drivers/virtio/virtio_ring.c | 27 ++++++++++++++-- include/linux/virtio.h | 1 + include/linux/virtio_config.h | 6 ++++ 9 files changed, 118 insertions(+), 31 deletions(-) -- 2.25.1
If an untrusted device neogitates BLK_F_MQ but advertises a zero num_queues, the driver may end up trying to allocating zero size buffers where ZERO_SIZE_PTR is returned which may pass the checking against the NULL. This will lead unexpected results. Fixing this by using single queue if num_queues is zero. Cc: Paolo Bonzini <pbonzini at redhat.com> Cc: Stefan Hajnoczi <stefanha at redhat.com> Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/block/virtio_blk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index e574fbf5e6df..f130d12df4b9 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -498,7 +498,8 @@ static int init_vq(struct virtio_blk *vblk) err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ, struct virtio_blk_config, num_queues, &num_vqs); - if (err) + /* We need at least on virtqueue */ + if (err || !num_vqs) num_vqs = 1; num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs); -- 2.25.1
This patch adds doc for validate() method. Signed-off-by: Jason Wang <jasowang at redhat.com> --- include/linux/virtio.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 41edbc01ffa4..0cd8685aeba4 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -152,6 +152,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev); * @feature_table_size: number of entries in the feature table array. * @feature_table_legacy: same as feature_table but when working in legacy mode. * @feature_table_size_legacy: number of entries in feature table legacy array. + * @validate: optional function to do early checks * @probe: the function to call when a device is found. Returns 0 or -errno. * @scan: optional function to call after successful probe; intended * for virtio-scsi to invoke a scan. -- 2.25.1
This patch switches to use validate() to filter out the features that is not supported by the rproc. Cc: Amit Shah <amit at kernel.org> Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/char/virtio_console.c | 41 ++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 7eaf303a7a86..daeed31df622 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -1172,9 +1172,7 @@ static void resize_console(struct port *port) vdev = port->portdev->vdev; - /* Don't test F_SIZE at all if we're rproc: not a valid feature! */ - if (!is_rproc_serial(vdev) && - virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)) + if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)) hvc_resize(port->cons.hvc, port->cons.ws); } @@ -1981,6 +1979,29 @@ static void virtcons_remove(struct virtio_device *vdev) kfree(portdev); } +static int virtcons_validate(struct virtio_device *vdev) +{ + if (is_rproc_serial(vdev)) { + /* Don't test F_SIZE at all if we're rproc: not a + * valid feature! */ + __virtio_clear_bit(vdev, VIRTIO_CONSOLE_F_SIZE); + /* Don't test MULTIPORT at all if we're rproc: not a + * valid feature! */ + __virtio_clear_bit(vdev, VIRTIO_CONSOLE_F_MULTIPORT); + } + + /* We only need a config space if features are offered */ + if (!vdev->config->get && + (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE) + || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))) { + dev_err(&vdev->dev, "%s failure: config access disabled\n", + __func__); + return -EINVAL; + } + + return 0; +} + /* * Once we're further in boot, we get probed like any other virtio * device. @@ -1996,15 +2017,6 @@ static int virtcons_probe(struct virtio_device *vdev) bool multiport; bool early = early_put_chars != NULL; - /* We only need a config space if features are offered */ - if (!vdev->config->get && - (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE) - || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))) { - dev_err(&vdev->dev, "%s failure: config access disabled\n", - __func__); - return -EINVAL; - } - /* Ensure to read early_put_chars now */ barrier(); @@ -2031,9 +2043,7 @@ static int virtcons_probe(struct virtio_device *vdev) multiport = false; portdev->max_nr_ports = 1; - /* Don't test MULTIPORT at all if we're rproc: not a valid feature! */ - if (!is_rproc_serial(vdev) && - virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT, + if (virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT, struct virtio_console_config, max_nr_ports, &portdev->max_nr_ports) == 0) { multiport = true; @@ -2210,6 +2220,7 @@ static struct virtio_driver virtio_console = { .driver.name = KBUILD_MODNAME, .driver.owner = THIS_MODULE, .id_table = id_table, + .validate = virtcons_validate, .probe = virtcons_probe, .remove = virtcons_remove, .config_changed = config_intr, -- 2.25.1
Jason Wang
2021-Sep-13 05:53 UTC
[PATCH 4/9] virtio_console: validate max_nr_ports before trying to use it
We calculate nr_ports based on the max_nr_ports: nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2; If the device advertises a large max_nr_ports, we will end up with a integer overflow. Fixing this by validating the max_nr_ports advertised by the device in .validate() and clear the MULTIPORT is it's greater than 0x8000 (which is guaranteed be safe). Cc: Amit Shah <amit at kernel.org> Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/char/virtio_console.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index daeed31df622..ef13763699c0 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -28,6 +28,7 @@ #include "../tty/hvc/hvc_console.h" #define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC) +#define VIRTCONS_MAX_PORTS 0x8000 /* * This is a global struct for storing common data for all the devices @@ -1981,6 +1982,8 @@ static void virtcons_remove(struct virtio_device *vdev) static int virtcons_validate(struct virtio_device *vdev) { + u32 max_nr_ports; + if (is_rproc_serial(vdev)) { /* Don't test F_SIZE at all if we're rproc: not a * valid feature! */ @@ -1999,6 +2002,13 @@ static int virtcons_validate(struct virtio_device *vdev) return -EINVAL; } + if (virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT, + struct virtio_console_config, max_nr_ports, + &max_nr_ports) == 0) { + if (max_nr_ports == 0 || max_nr_ports > VIRTCONS_MAX_PORTS) + __virtio_clear_bit(vdev, VIRTIO_CONSOLE_F_MULTIPORT); + } + return 0; } -- 2.25.1
Signed-off-by: Jason Wang <jasowang at redhat.com> --- include/linux/virtio_config.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index 8519b3ae5d52..f2891c6221a1 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -23,6 +23,8 @@ struct virtio_shm_region { * any of @get/@set, @get_status/@set_status, or @get_features/ * @finalize_features are NOT safe to be called from an atomic * context. + * @ready: make the device ready + * vdev: the virtio_device * @get: read the value of a configuration field * vdev: the virtio_device * offset: the offset of the configuration field @@ -75,6 +77,7 @@ struct virtio_shm_region { */ typedef void vq_callback_t(struct virtqueue *); struct virtio_config_ops { + void (*ready)(struct virtio_device *vdev); void (*get)(struct virtio_device *vdev, unsigned offset, void *buf, unsigned len); void (*set)(struct virtio_device *vdev, unsigned offset, @@ -229,6 +232,9 @@ void virtio_device_ready(struct virtio_device *dev) { unsigned status = dev->config->get_status(dev); + if (dev->config->ready) + dev->config->ready(dev); + BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK); } -- 2.25.1
We used to synchronize pending MSI-X irq handlers via synchronize_irq(), this may not work for the untrusted device which may keep sending interrupts after reset which may lead unexpected results. Similarly, we should not enable MSI-X interrupt until the device is ready. So this patch fixes those two issues by: 1) switching to use disable_irq() to prevent the virtio interrupt handlers to be called after the device is reset. 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready() This can make sure the virtio interrupt handler won't be called before virtio_device_ready() and after reset. Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/virtio/virtio_pci_common.c | 27 +++++++++++++++++++++------ drivers/virtio/virtio_pci_common.h | 6 ++++-- drivers/virtio/virtio_pci_legacy.c | 5 +++-- drivers/virtio/virtio_pci_modern.c | 6 ++++-- 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index b35bb2d57f62..0b9523e6dd39 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy, "Force legacy mode for transitional virtio 1 devices"); #endif -/* wait for pending irq handlers */ -void vp_synchronize_vectors(struct virtio_device *vdev) +/* disable irq handlers */ +void vp_disable_vectors(struct virtio_device *vdev) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); int i; @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev) synchronize_irq(vp_dev->pci_dev->irq); for (i = 0; i < vp_dev->msix_vectors; ++i) - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); + disable_irq(pci_irq_vector(vp_dev->pci_dev, i)); +} + +/* enable irq handlers */ +void vp_enable_vectors(struct virtio_device *vdev) +{ + struct virtio_pci_device *vp_dev = to_vp_device(vdev); + int i; + + if (vp_dev->intx_enabled) + return; + + for (i = 0; i < vp_dev->msix_vectors; ++i) + enable_irq(pci_irq_vector(vp_dev->pci_dev, i)); } /* the notify function used when creating a virt queue */ @@ -141,7 +154,8 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors, snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names, "%s-config", name); err = request_irq(pci_irq_vector(vp_dev->pci_dev, v), - vp_config_changed, 0, vp_dev->msix_names[v], + vp_config_changed, IRQF_NO_AUTOEN, + vp_dev->msix_names[v], vp_dev); if (err) goto error; @@ -160,7 +174,8 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors, snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names, "%s-virtqueues", name); err = request_irq(pci_irq_vector(vp_dev->pci_dev, v), - vp_vring_interrupt, 0, vp_dev->msix_names[v], + vp_vring_interrupt, IRQF_NO_AUTOEN, + vp_dev->msix_names[v], vp_dev); if (err) goto error; @@ -337,7 +352,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs, "%s-%s", dev_name(&vp_dev->vdev.dev), names[i]); err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec), - vring_interrupt, 0, + vring_interrupt, IRQF_NO_AUTOEN, vp_dev->msix_names[msix_vec], vqs[i]); if (err) diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h index beec047a8f8d..a235ce9ff6a5 100644 --- a/drivers/virtio/virtio_pci_common.h +++ b/drivers/virtio/virtio_pci_common.h @@ -102,8 +102,10 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) return container_of(vdev, struct virtio_pci_device, vdev); } -/* wait for pending irq handlers */ -void vp_synchronize_vectors(struct virtio_device *vdev); +/* disable irq handlers */ +void vp_disable_vectors(struct virtio_device *vdev); +/* enable irq handlers */ +void vp_enable_vectors(struct virtio_device *vdev); /* the notify function used when creating a virt queue */ bool vp_notify(struct virtqueue *vq); /* the config->del_vqs() implementation */ diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c index d62e9835aeec..bdf6bc667ab5 100644 --- a/drivers/virtio/virtio_pci_legacy.c +++ b/drivers/virtio/virtio_pci_legacy.c @@ -97,8 +97,8 @@ static void vp_reset(struct virtio_device *vdev) /* Flush out the status write, and flush in device writes, * including MSi-X interrupts, if any. */ ioread8(vp_dev->ioaddr + VIRTIO_PCI_STATUS); - /* Flush pending VQ/configuration callbacks. */ - vp_synchronize_vectors(vdev); + /* Disable VQ/configuration callbacks. */ + vp_disable_vectors(vdev); } static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) @@ -194,6 +194,7 @@ static void del_vq(struct virtio_pci_vq_info *info) } static const struct virtio_config_ops virtio_pci_config_ops = { + .ready = vp_enable_vectors, .get = vp_get, .set = vp_set, .get_status = vp_get_status, diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c index 30654d3a0b41..acf0f6b6381d 100644 --- a/drivers/virtio/virtio_pci_modern.c +++ b/drivers/virtio/virtio_pci_modern.c @@ -172,8 +172,8 @@ static void vp_reset(struct virtio_device *vdev) */ while (vp_modern_get_status(mdev)) msleep(1); - /* Flush pending VQ/configuration callbacks. */ - vp_synchronize_vectors(vdev); + /* Disable VQ/configuration callbacks. */ + vp_disable_vectors(vdev); } static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) @@ -380,6 +380,7 @@ static bool vp_get_shm_region(struct virtio_device *vdev, } static const struct virtio_config_ops virtio_pci_config_nodev_ops = { + .ready = vp_enable_vectors, .get = NULL, .set = NULL, .generation = vp_generation, @@ -397,6 +398,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = { }; static const struct virtio_config_ops virtio_pci_config_ops = { + .ready = vp_enable_vectors, .get = vp_get, .set = vp_set, .generation = vp_generation, -- 2.25.1
This patch tries to make sure the virtio interrupt handler for INTX won't be called after a reset and before virtio_device_ready(). We can't use IRQF_NO_AUTOEN since we're using shared interrupt (IRQF_SHARED). So this patch tracks the INTX enabling status in a new intx_soft_enabled variable and toggle it during in vp_disable/enable_vectors(). The INTX interrupt handler will check intx_soft_enabled before processing the actual interrupt. Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/virtio/virtio_pci_common.c | 18 ++++++++++++++++-- drivers/virtio/virtio_pci_common.h | 1 + 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index 0b9523e6dd39..835197151dc1 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -30,8 +30,12 @@ void vp_disable_vectors(struct virtio_device *vdev) struct virtio_pci_device *vp_dev = to_vp_device(vdev); int i; - if (vp_dev->intx_enabled) + if (vp_dev->intx_enabled) { + vp_dev->intx_soft_enabled = false; + /* ensure the vp_interrupt see this intx_soft_enabled value */ + smp_wmb(); synchronize_irq(vp_dev->pci_dev->irq); + } for (i = 0; i < vp_dev->msix_vectors; ++i) disable_irq(pci_irq_vector(vp_dev->pci_dev, i)); @@ -43,8 +47,12 @@ void vp_enable_vectors(struct virtio_device *vdev) struct virtio_pci_device *vp_dev = to_vp_device(vdev); int i; - if (vp_dev->intx_enabled) + if (vp_dev->intx_enabled) { + vp_dev->intx_soft_enabled = true; + /* ensure the vp_interrupt see this intx_soft_enabled value */ + smp_wmb(); return; + } for (i = 0; i < vp_dev->msix_vectors; ++i) enable_irq(pci_irq_vector(vp_dev->pci_dev, i)); @@ -97,6 +105,12 @@ static irqreturn_t vp_interrupt(int irq, void *opaque) struct virtio_pci_device *vp_dev = opaque; u8 isr; + if (!vp_dev->intx_soft_enabled) + return IRQ_NONE; + + /* read intx_soft_enabled before read others */ + smp_rmb(); + /* reading the ISR has the effect of also clearing it so it's very * important to save off the value. */ isr = ioread8(vp_dev->isr); diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h index a235ce9ff6a5..3c06e0f92ee4 100644 --- a/drivers/virtio/virtio_pci_common.h +++ b/drivers/virtio/virtio_pci_common.h @@ -64,6 +64,7 @@ struct virtio_pci_device { /* MSI-X support */ int msix_enabled; int intx_enabled; + bool intx_soft_enabled; cpumask_var_t *msix_affinity_masks; /* Name strings for interrupts. This size should be enough, * and I'm too lazy to allocate each name separately. */ -- 2.25.1
We're actually tracking descriptor address and length instead of the buffer. Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/virtio/virtio_ring.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index dd95dfd85e98..d2ca0a7365f8 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -79,8 +79,8 @@ struct vring_desc_state_packed { }; struct vring_desc_extra { - dma_addr_t addr; /* Buffer DMA addr. */ - u32 len; /* Buffer length. */ + dma_addr_t addr; /* Descriptor DMA addr. */ + u32 len; /* Descriptor length. */ u16 flags; /* Descriptor flags. */ u16 next; /* The next desc state in a list. */ }; -- 2.25.1
This patch validate the used buffer length provided by the device before trying to use it. This is done by record the in buffer length in a new field in desc_state structure during virtqueue_add(), then we can fail the virtqueue_get_buf() when we find the device is trying to give us a used buffer length which is greater than the in buffer length. Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/virtio/virtio_ring.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index d2ca0a7365f8..b8374a6144f3 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -69,6 +69,7 @@ struct vring_desc_state_split { void *data; /* Data for callback. */ struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ + u64 buflen; /* In buffer length */ }; struct vring_desc_state_packed { @@ -76,6 +77,7 @@ struct vring_desc_state_packed { struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */ u16 num; /* Descriptor list length. */ u16 last; /* The last desc state in a list. */ + u64 buflen; /* In buffer length */ }; struct vring_desc_extra { @@ -490,6 +492,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, unsigned int i, n, avail, descs_used, prev, err_idx; int head; bool indirect; + u64 buflen = 0; START_USE(vq); @@ -571,6 +574,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, VRING_DESC_F_NEXT | VRING_DESC_F_WRITE, indirect); + buflen += sg->length; } } /* Last one doesn't continue. */ @@ -605,6 +609,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, /* Store token and indirect buffer state. */ vq->split.desc_state[head].data = data; + vq->split.desc_state[head].buflen = buflen; if (indirect) vq->split.desc_state[head].indir_desc = desc; else @@ -784,6 +789,11 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, BAD_RING(vq, "id %u is not a head!\n", i); return NULL; } + if (unlikely(*len > vq->split.desc_state[i].buflen)) { + BAD_RING(vq, "used len %d is larger than in buflen %lld\n", + *len, vq->split.desc_state[i].buflen); + return NULL; + } /* detach_buf_split clears data, so grab it now. */ ret = vq->split.desc_state[i].data; @@ -1062,6 +1072,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, unsigned int i, n, err_idx; u16 head, id; dma_addr_t addr; + u64 buflen = 0; head = vq->packed.next_avail_idx; desc = alloc_indirect_packed(total_sg, gfp); @@ -1089,6 +1100,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, desc[i].addr = cpu_to_le64(addr); desc[i].len = cpu_to_le32(sg->length); i++; + if (n >= out_sgs) + buflen += sg->length; } } @@ -1141,6 +1154,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq, vq->packed.desc_state[id].data = data; vq->packed.desc_state[id].indir_desc = desc; vq->packed.desc_state[id].last = id; + vq->packed.desc_state[id].buflen = buflen; vq->num_added += 1; @@ -1176,6 +1190,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, unsigned int i, n, c, descs_used, err_idx; __le16 head_flags, flags; u16 head, id, prev, curr, avail_used_flags; + u64 buflen = 0; START_USE(vq); @@ -1250,6 +1265,8 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, 1 << VRING_PACKED_DESC_F_AVAIL | 1 << VRING_PACKED_DESC_F_USED; } + if (n >= out_sgs) + buflen += sg->length; } } @@ -1268,6 +1285,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq, vq->packed.desc_state[id].data = data; vq->packed.desc_state[id].indir_desc = ctx; vq->packed.desc_state[id].last = prev; + vq->packed.desc_state[id].buflen = buflen; /* * A driver MUST NOT make the first descriptor in the list @@ -1455,6 +1473,11 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, BAD_RING(vq, "id %u is not a head!\n", id); return NULL; } + if (unlikely(*len > vq->packed.desc_state[id].buflen)) { + BAD_RING(vq, "used len %d is larger than in buflen %lld\n", + *len, vq->packed.desc_state[id].buflen); + return NULL; + } /* detach_buf_packed clears data, so grab it now. */ ret = vq->packed.desc_state[id].data; -- 2.25.1
On Mon, Sep 13, 2021 at 01:53:44PM +0800, Jason Wang wrote:> Hi All: > > This series treis to do more hardening for virito. > > patch 1 validates the num_queues for virio-blk device. > patch 2-4 validates max_nr_ports for virito-console device. > patch 5-7 harden virtio-pci interrupts to make sure no exepcted > interrupt handler is tiggered. If this makes sense we can do similar > things in other transport drivers. > patch 8-9 validate used ring length. > > Smoking test on blk/net with packed=on/off and iommu_platform=on/off. > > Please review. > > ThanksSo I poked at console at least, and I think I see an issue: if interrupt handler queues a work/bh, then it can still run while reset is in progress. I sent a patch to fix it for console removal specifically, but I suspect it's not enough e.g. freeze is still broken. And note this has been reported without any TDX things - it's not a malicious device issue, can be triggered just by module unload. I am vaguely thinking about new APIs to disable/enable callbacks. An alternative: 1. adding new remove_nocb/freeze_nocb calls 2. disabling/enabling interrupts automatically around these 3. gradually moving devices to using these 4. once/if all device move, removing the old callbacks the advantage here is that we'll be sure calls are always paired correctly.> Jason Wang (9): > virtio-blk: validate num_queues during probe > virtio: add doc for validate() method > virtio-console: switch to use .validate() > virtio_console: validate max_nr_ports before trying to use it > virtio_config: introduce a new ready method > virtio_pci: harden MSI-X interrupts > virtio-pci: harden INTX interrupts > virtio_ring: fix typos in vring_desc_extra > virtio_ring: validate used buffer length > > drivers/block/virtio_blk.c | 3 +- > drivers/char/virtio_console.c | 51 +++++++++++++++++++++--------- > drivers/virtio/virtio_pci_common.c | 43 +++++++++++++++++++++---- > drivers/virtio/virtio_pci_common.h | 7 ++-- > drivers/virtio/virtio_pci_legacy.c | 5 +-- > drivers/virtio/virtio_pci_modern.c | 6 ++-- > drivers/virtio/virtio_ring.c | 27 ++++++++++++++-- > include/linux/virtio.h | 1 + > include/linux/virtio_config.h | 6 ++++ > 9 files changed, 118 insertions(+), 31 deletions(-) > > -- > 2.25.1