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