[PATCH 0/9] More virtio hardening

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Jason Wang <redacted>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

This patch adds doc for validate() method.

Signed-off-by: Jason Wang <redacted>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

This patch switches to use validate() to filter out the features that
is not supported by the rproc.

Cc: Amit Shah <amit@kernel.org>
Signed-off-by: Jason Wang <redacted>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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@kernel.org>
Signed-off-by: Jason Wang <redacted>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Signed-off-by: Jason Wang <redacted>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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 <redacted>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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 <redacted>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

We're actually tracking descriptor address and length instead of the
buffer.

Signed-off-by: Jason Wang <redacted>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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 <redacted>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:

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 <redacted>

I don't get the threat model here. Isn't disabling irqs done by the 
hypervisor anyway? Is there a reason to trust disable_irq but not
device reset?

Cc a bunch more people ...

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin [off-list ref] wrote:

On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:

quoted

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 <redacted>

I don't get the threat model here. Isn't disabling irqs done by the
hypervisor anyway? Is there a reason to trust disable_irq but not
device reset?

My understanding is that e.g in the case of SEV/TDX we don't trust the
hypervisor. So the hypervisor can keep sending interrupts even if the
device is reset. The guest can only trust its own software interrupt
management logic to avoid call virtio callback in this case.

Thanks

Cc a bunch more people ...

quoted

---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:

On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:

quoted

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 <redacted>

I don't get the threat model here. Isn't disabling irqs done by the
hypervisor anyway? Is there a reason to trust disable_irq but not
device reset?

My understanding is that e.g in the case of SEV/TDX we don't trust the
hypervisor. So the hypervisor can keep sending interrupts even if the
device is reset. The guest can only trust its own software interrupt
management logic to avoid call virtio callback in this case.

Thanks

Hmm but I don't see how do these patches do this.
They call disable_irq but can't the hypervisor keep
sending interrupts after disable_irq, too?

quoted

Cc a bunch more people ...

quoted

---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Mon, Sep 13, 2021 at 01:53:51PM +0800, Jason Wang wrote:

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 <redacted>

Not all that excited about all the memory barriers for something
that should be an extremely rare event (for most kernels -
literally once per boot). Can't we do better?

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Mon, Sep 13, 2021 at 2:28 PM Michael S. Tsirkin [off-list ref] wrote:

On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:

quoted

On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:

quoted

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 <redacted>

I don't get the threat model here. Isn't disabling irqs done by the
hypervisor anyway? Is there a reason to trust disable_irq but not
device reset?

My understanding is that e.g in the case of SEV/TDX we don't trust the
hypervisor. So the hypervisor can keep sending interrupts even if the
device is reset. The guest can only trust its own software interrupt
management logic to avoid call virtio callback in this case.

Thanks

Hmm but I don't see how do these patches do this.
They call disable_irq but can't the hypervisor keep
sending interrupts after disable_irq, too?

Yes, but since the irq is disabled, the vring or config callback won't
be called in this case.

Thanks

quoted

quoted

Cc a bunch more people ...

quoted

---
 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