[PATCH V2 00/12] More virtio hardening

[PATCH V2 00/12] More virtio hardening

[Jason Wang]

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.
patch 9-11 let the driver to validate the used length instead of the
virtio core

Smoking test on blk/net with packed=on/off and iommu_platform=on/off.

Please review.

Changes since V1:
- fix and document the memory ordering around the intx_soft_enabled
  when enabling and disabling INTX interrupt
- for the driver that can validate the used length, virtio core won't
  even try to allocate auxilary arrays and validate the used length
- tweak the commit log
- fix typos

Jason Wang (12):
  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
  virtio-net: don't let virtio core to validate used length
  virtio-blk: don't let virtio core to validate used length
  virtio-scsi: don't let virtio core to validate used buffer length

 drivers/block/virtio_blk.c         |  4 +-
 drivers/char/virtio_console.c      | 51 +++++++++++++++++--------
 drivers/net/virtio_net.c           |  1 +
 drivers/scsi/virtio_scsi.c         |  1 +
 drivers/virtio/virtio_pci_common.c | 49 ++++++++++++++++++++----
 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       | 60 +++++++++++++++++++++++++++++-
 include/linux/virtio.h             |  3 ++
 include/linux/virtio_config.h      |  6 +++
 11 files changed, 162 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH V2 01/12] virtio-blk: validate num_queues during probe

[Jason Wang]

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>
Cc: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Jason Wang <jasowang@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 9b3bd083b411..9deff01a38cb 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -495,7 +495,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 one virtqueue */
+	if (err || !num_vqs)
 		num_vqs = 1;
 
 	num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
-- 
2.25.1

[PATCH V2 02/12] virtio: add doc for validate() method

[Jason Wang]

This patch adds doc for validate() method.

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

[PATCH V2 03/12] virtio-console: switch to use .validate()

[Jason Wang]

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

[PATCH V2 04/12] virtio_console: validate max_nr_ports before trying to use it

[Jason Wang]

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

[PATCH V2 05/12] virtio_config: introduce a new ready method

[Jason Wang]

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

[PATCH V2 06/12] virtio_pci: harden MSI-X interrupts

[Jason Wang]

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.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Jason Wang <jasowang@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

[PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Jason Wang]

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.

Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
 drivers/virtio/virtio_pci_common.h |  1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 0b9523e6dd39..5ae6a2a4eb77 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -30,8 +30,16 @@ 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) {
+		/*
+		 * The below synchronize() guarantees that any
+		 * interrupt for this line arriving after
+		 * synchronize_irq() has completed is guaranteed to see
+		 * intx_soft_enabled == false.
+		 */
+		WRITE_ONCE(vp_dev->intx_soft_enabled, false);
 		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 +51,16 @@ 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) {
+		disable_irq(vp_dev->pci_dev->irq);
+		/*
+		 * The above disable_irq() provides TSO ordering and
+		 * as such promotes the below store to store-release.
+		 */
+		WRITE_ONCE(vp_dev->intx_soft_enabled, true);
+		enable_irq(vp_dev->pci_dev->irq);
 		return;
+	}
 
 	for (i = 0; i < vp_dev->msix_vectors; ++i)
 		enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
@@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
 	struct virtio_pci_device *vp_dev = opaque;
 	u8 isr;
 
+	/* read intx_soft_enabled before read others */
+	if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
+		return IRQ_NONE;
+
 	/* 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

[PATCH V2 08/12] virtio_ring: fix typos in vring_desc_extra

[Jason Wang]

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

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

[PATCH V2 09/12] virtio_ring: validate used buffer length

[Jason Wang]

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.

Since some drivers have already done the validation by itself, this
patch tries to makes the core validation optional. For the driver that
doesn't want the validation, it can set the validate_used to be
true (which could be overridden by force_used_validation). To be more
efficient, a dedicate array is used for storing the validate used
length, this helps to eliminate the cache stress if validation is done
by the driver.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 56 ++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h       |  2 ++
 2 files changed, 58 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index d2ca0a7365f8..dee962bd8b04 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -14,6 +14,9 @@
 #include <linux/spinlock.h>
 #include <xen/xen.h>
 
+static bool force_used_validation = false;
+module_param(force_used_validation, bool, 0444);
+
 #ifdef DEBUG
 /* For development, we want to crash whenever the ring is screwed. */
 #define BAD_RING(_vq, fmt, args...)				\
@@ -182,6 +185,9 @@ struct vring_virtqueue {
 		} packed;
 	};
 
+	/* Per-descriptor in buffer length */
+	u32 *buflen;
+
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	bool (*notify)(struct virtqueue *vq);
 
@@ -490,6 +496,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 	unsigned int i, n, avail, descs_used, prev, err_idx;
 	int head;
 	bool indirect;
+	u32 buflen = 0;
 
 	START_USE(vq);
 
@@ -571,6 +578,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. */
@@ -610,6 +618,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 	else
 		vq->split.desc_state[head].indir_desc = ctx;
 
+	/* Store in buffer length if necessary */
+	if (vq->buflen)
+		vq->buflen[head] = buflen;
+
 	/* Put entry in available array (but don't update avail->idx until they
 	 * do sync). */
 	avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1);
@@ -784,6 +796,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 (vq->buflen && unlikely(*len > vq->buflen[i])) {
+		BAD_RING(vq, "used len %d is larger than in buflen %u\n",
+			*len, vq->buflen[i]);
+		return NULL;
+	}
 
 	/* detach_buf_split clears data, so grab it now. */
 	ret = vq->split.desc_state[i].data;
@@ -1062,6 +1079,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 	unsigned int i, n, err_idx;
 	u16 head, id;
 	dma_addr_t addr;
+	u32 buflen = 0;
 
 	head = vq->packed.next_avail_idx;
 	desc = alloc_indirect_packed(total_sg, gfp);
@@ -1089,6 +1107,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;
 		}
 	}
 
@@ -1142,6 +1162,10 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 	vq->packed.desc_state[id].indir_desc = desc;
 	vq->packed.desc_state[id].last = id;
 
+	/* Store in buffer length if necessary */
+	if (vq->buflen)
+		vq->buflen[id] = buflen;
+
 	vq->num_added += 1;
 
 	pr_debug("Added buffer head %i to %p\n", head, vq);
@@ -1176,6 +1200,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;
+	u32 buflen = 0;
 
 	START_USE(vq);
 
@@ -1250,6 +1275,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;
 		}
 	}
 
@@ -1269,6 +1296,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 	vq->packed.desc_state[id].indir_desc = ctx;
 	vq->packed.desc_state[id].last = prev;
 
+	/* Store in buffer length if necessary */
+	if (vq->buflen)
+		vq->buflen[id] = buflen;
+
 	/*
 	 * A driver MUST NOT make the first descriptor in the list
 	 * available before all subsequent descriptors comprising
@@ -1455,6 +1486,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 (vq->buflen && unlikely(*len > vq->buflen[id])) {
+		BAD_RING(vq, "used len %d is larger than in buflen %u\n",
+			*len, vq->buflen[id]);
+		return NULL;
+	}
 
 	/* detach_buf_packed clears data, so grab it now. */
 	ret = vq->packed.desc_state[id].data;
@@ -1660,6 +1696,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	struct vring_virtqueue *vq;
 	struct vring_packed_desc *ring;
 	struct vring_packed_desc_event *driver, *device;
+	struct virtio_driver *drv = drv_to_virtio(vdev->dev.driver);
 	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
 	size_t ring_size_in_bytes, event_size_in_bytes;
 
@@ -1749,6 +1786,13 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	if (!vq->packed.desc_extra)
 		goto err_desc_extra;
 
+	if (!drv->validate_used || force_used_validation) {
+		vq->buflen = kmalloc_array(num, sizeof(*vq->buflen),
+					   GFP_KERNEL);
+		if (!vq->buflen)
+			goto err_buflen;
+	}
+
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback) {
 		vq->packed.event_flags_shadow = VRING_PACKED_EVENT_FLAG_DISABLE;
@@ -1761,6 +1805,8 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	spin_unlock(&vdev->vqs_list_lock);
 	return &vq->vq;
 
+err_buflen:
+	kfree(vq->packed.desc_extra);
 err_desc_extra:
 	kfree(vq->packed.desc_state);
 err_desc_state:
@@ -2168,6 +2214,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 					void (*callback)(struct virtqueue *),
 					const char *name)
 {
+	struct virtio_driver *drv = drv_to_virtio(vdev->dev.driver);
 	struct vring_virtqueue *vq;
 
 	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
@@ -2227,6 +2274,13 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	if (!vq->split.desc_extra)
 		goto err_extra;
 
+	if (!drv->validate_used || force_used_validation) {
+		vq->buflen = kmalloc_array(vring.num, sizeof(*vq->buflen),
+					   GFP_KERNEL);
+		if (!vq->buflen)
+			goto err_buflen;
+	}
+
 	/* Put everything in free lists. */
 	vq->free_head = 0;
 	memset(vq->split.desc_state, 0, vring.num *
@@ -2237,6 +2291,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	spin_unlock(&vdev->vqs_list_lock);
 	return &vq->vq;
 
+err_buflen:
+	kfree(vq->split.desc_extra);
 err_extra:
 	kfree(vq->split.desc_state);
 err_state:
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 0cd8685aeba4..79e4f6765e3a 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -153,6 +153,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev);
  * @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
+ * @validate_used: whether the driver can validate used buffer length
  * @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.
@@ -169,6 +170,7 @@ struct virtio_driver {
 	unsigned int feature_table_size;
 	const unsigned int *feature_table_legacy;
 	unsigned int feature_table_size_legacy;
+	bool validate_used;
 	int (*validate)(struct virtio_device *dev);
 	int (*probe)(struct virtio_device *dev);
 	void (*scan)(struct virtio_device *dev);
-- 
2.25.1

[PATCH V2 10/12] virtio-net: don't let virtio core to validate used length

[Jason Wang]

For RX virtuqueue, the used length is validated in all the three paths
(big, small and mergeable). For control vq, we never tries to use used
length. So this patch forbids the core to validate the used length.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 79bd2585ec6b..f2f9ea167020 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3384,6 +3384,7 @@ static struct virtio_driver virtio_net_driver = {
 	.feature_table_size = ARRAY_SIZE(features),
 	.feature_table_legacy = features_legacy,
 	.feature_table_size_legacy = ARRAY_SIZE(features_legacy),
+	.validate_used = true,
 	.driver.name =	KBUILD_MODNAME,
 	.driver.owner =	THIS_MODULE,
 	.id_table =	id_table,
-- 
2.25.1

[PATCH V2 11/12] virtio-blk: don't let virtio core to validate used length

[Jason Wang]

We never tries to use used length, so the patch prevents the virtio
core from validating used length.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/block/virtio_blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 9deff01a38cb..12f95fb6967e 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -1007,6 +1007,7 @@ static struct virtio_driver virtio_blk = {
 	.feature_table_size		= ARRAY_SIZE(features),
 	.feature_table_legacy		= features_legacy,
 	.feature_table_size_legacy	= ARRAY_SIZE(features_legacy),
+	.validate_used			= true,
 	.driver.name			= KBUILD_MODNAME,
 	.driver.owner			= THIS_MODULE,
 	.id_table			= id_table,
-- 
2.25.1

[PATCH V2 12/12] virtio-scsi: don't let virtio core to validate used buffer length

[Jason Wang]

We never tries to use used length, so the patch prevents the virtio
core from validating used length.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/scsi/virtio_scsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index c25ce8f0e0af..2af33fc36f8a 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -977,6 +977,7 @@ static unsigned int features[] = {
 static struct virtio_driver virtio_scsi_driver = {
 	.feature_table = features,
 	.feature_table_size = ARRAY_SIZE(features),
+	.validate_used = true,
 	.driver.name = KBUILD_MODNAME,
 	.driver.owner = THIS_MODULE,
 	.id_table = id_table,
-- 
2.25.1

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 02:52:22PM +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.
> 
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
>  drivers/virtio/virtio_pci_common.h |  1 +
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 0b9523e6dd39..5ae6a2a4eb77 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -30,8 +30,16 @@ 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) {
> +		/*
> +		 * The below synchronize() guarantees that any
> +		 * interrupt for this line arriving after
> +		 * synchronize_irq() has completed is guaranteed to see
> +		 * intx_soft_enabled == false.
> +		 */
> +		WRITE_ONCE(vp_dev->intx_soft_enabled, false);
>  		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 +51,16 @@ 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) {
> +		disable_irq(vp_dev->pci_dev->irq);
> +		/*
> +		 * The above disable_irq() provides TSO ordering and
> +		 * as such promotes the below store to store-release.
> +		 */
> +		WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> +		enable_irq(vp_dev->pci_dev->irq);
>  		return;
> +	}
>  
>  	for (i = 0; i < vp_dev->msix_vectors; ++i)
>  		enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
>  	struct virtio_pci_device *vp_dev = opaque;
>  	u8 isr;
>  
> +	/* read intx_soft_enabled before read others */
> +	if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> +		return IRQ_NONE;
> +
>  	/* 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);

I don't see why we need this ordering guarantee here.

synchronize_irq above makes sure no interrupt handler
is in progress. the handler itself thus does not need
any specific order, it is ok if intx_soft_enabled is read
after, not before the rest of it.

Just READ_ONCE should be enough, and we can drop the comment.


> 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

Re: [PATCH V2 03/12] virtio-console: switch to use .validate()

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 02:52:18PM +0800, Jason Wang wrote:
> This patch switches to use validate() to filter out the features that
> is not supported by the rproc.

are not supported

> 
> Cc: Amit Shah <amit@kernel.org>
> Signed-off-by: Jason Wang <jasowang@redhat.com>


Does this have anything to do with hardening?

It seems cleaner to not negotiate features we do not use,
but given we did this for many years ... should we bother
at this point?


> ---
>  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! */


This comment needs to be fixed now. And the format's wrong
since you made it a multi-line comment.
Should be
	/*
	 * like
	 * this
	 */

> +		__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.

This switches the order of tests around, so if an rproc device
offers VIRTIO_CONSOLE_F_SIZE or VIRTIO_CONSOLE_F_MULTIPORT
without get it will now try to work instead of failing.

Which is maybe a worthy goal, but given rproc does not support
virtio 1.0 it also risks trying to drive something completely
unreasonable.

Overall does not feel like hardening which is supposed to make
things more strict, not less.


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

Re: [PATCH V2 05/12] virtio_config: introduce a new ready method

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 02:52:20PM +0800, Jason Wang wrote:
> Signed-off-by: Jason Wang <jasowang@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

I think it's too vague. device_ready makes device ready to receive
kicks. What does this one do? I don't like calling things by where
they are called from, I prefer calling them by what they do.
It actually enables callbacks, right?
So enable_cbs?


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

Re: [PATCH V2 06/12] virtio_pci: harden MSI-X interrupts

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 02:52:21PM +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.

You mean until the driver 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.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Jason Wang <jasowang@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

Re: [PATCH V2 09/12] virtio_ring: validate used buffer length

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 02:52:24PM +0800, Jason Wang wrote:
> 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.
> 
> Since some drivers have already done the validation by itself, this

by themselves

> patch tries to makes the core validation optional. For the driver that
> doesn't want the validation, it can set the validate_used to be
> true (which could be overridden by force_used_validation). To be more
> efficient, a dedicate array is used for storing the validate used
> length, this helps to eliminate the cache stress if validation is done
> by the driver.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio_ring.c | 56 ++++++++++++++++++++++++++++++++++++
>  include/linux/virtio.h       |  2 ++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index d2ca0a7365f8..dee962bd8b04 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -14,6 +14,9 @@
>  #include <linux/spinlock.h>
>  #include <xen/xen.h>
>  
> +static bool force_used_validation = false;
> +module_param(force_used_validation, bool, 0444);
> +
>  #ifdef DEBUG
>  /* For development, we want to crash whenever the ring is screwed. */
>  #define BAD_RING(_vq, fmt, args...)				\
> @@ -182,6 +185,9 @@ struct vring_virtqueue {
>  		} packed;
>  	};
>  
> +	/* Per-descriptor in buffer length */
> +	u32 *buflen;
> +
>  	/* How to notify other side. FIXME: commonalize hcalls! */
>  	bool (*notify)(struct virtqueue *vq);
>  
> @@ -490,6 +496,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  	unsigned int i, n, avail, descs_used, prev, err_idx;
>  	int head;
>  	bool indirect;
> +	u32 buflen = 0;
>  
>  	START_USE(vq);
>  
> @@ -571,6 +578,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. */
> @@ -610,6 +618,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  	else
>  		vq->split.desc_state[head].indir_desc = ctx;
>  
> +	/* Store in buffer length if necessary */
> +	if (vq->buflen)
> +		vq->buflen[head] = buflen;
> +
>  	/* Put entry in available array (but don't update avail->idx until they
>  	 * do sync). */
>  	avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1);
> @@ -784,6 +796,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 (vq->buflen && unlikely(*len > vq->buflen[i])) {
> +		BAD_RING(vq, "used len %d is larger than in buflen %u\n",
> +			*len, vq->buflen[i]);
> +		return NULL;
> +	}
>  
>  	/* detach_buf_split clears data, so grab it now. */
>  	ret = vq->split.desc_state[i].data;
> @@ -1062,6 +1079,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  	unsigned int i, n, err_idx;
>  	u16 head, id;
>  	dma_addr_t addr;
> +	u32 buflen = 0;
>  
>  	head = vq->packed.next_avail_idx;
>  	desc = alloc_indirect_packed(total_sg, gfp);
> @@ -1089,6 +1107,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;
>  		}
>  	}
>  
> @@ -1142,6 +1162,10 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  	vq->packed.desc_state[id].indir_desc = desc;
>  	vq->packed.desc_state[id].last = id;
>  
> +	/* Store in buffer length if necessary */
> +	if (vq->buflen)
> +		vq->buflen[id] = buflen;
> +
>  	vq->num_added += 1;
>  
>  	pr_debug("Added buffer head %i to %p\n", head, vq);
> @@ -1176,6 +1200,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;
> +	u32 buflen = 0;
>  
>  	START_USE(vq);
>  
> @@ -1250,6 +1275,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;
>  		}
>  	}
>  
> @@ -1269,6 +1296,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	vq->packed.desc_state[id].indir_desc = ctx;
>  	vq->packed.desc_state[id].last = prev;
>  
> +	/* Store in buffer length if necessary */
> +	if (vq->buflen)
> +		vq->buflen[id] = buflen;
> +
>  	/*
>  	 * A driver MUST NOT make the first descriptor in the list
>  	 * available before all subsequent descriptors comprising
> @@ -1455,6 +1486,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 (vq->buflen && unlikely(*len > vq->buflen[id])) {
> +		BAD_RING(vq, "used len %d is larger than in buflen %u\n",
> +			*len, vq->buflen[id]);
> +		return NULL;
> +	}
>  
>  	/* detach_buf_packed clears data, so grab it now. */
>  	ret = vq->packed.desc_state[id].data;
> @@ -1660,6 +1696,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
>  	struct vring_virtqueue *vq;
>  	struct vring_packed_desc *ring;
>  	struct vring_packed_desc_event *driver, *device;
> +	struct virtio_driver *drv = drv_to_virtio(vdev->dev.driver);
>  	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
>  	size_t ring_size_in_bytes, event_size_in_bytes;
>  
> @@ -1749,6 +1786,13 @@ static struct virtqueue *vring_create_virtqueue_packed(
>  	if (!vq->packed.desc_extra)
>  		goto err_desc_extra;
>  
> +	if (!drv->validate_used || force_used_validation) {
> +		vq->buflen = kmalloc_array(num, sizeof(*vq->buflen),
> +					   GFP_KERNEL);
> +		if (!vq->buflen)
> +			goto err_buflen;
> +	}
> +
>  	/* No callback?  Tell other side not to bother us. */
>  	if (!callback) {
>  		vq->packed.event_flags_shadow = VRING_PACKED_EVENT_FLAG_DISABLE;
> @@ -1761,6 +1805,8 @@ static struct virtqueue *vring_create_virtqueue_packed(
>  	spin_unlock(&vdev->vqs_list_lock);
>  	return &vq->vq;
>  
> +err_buflen:
> +	kfree(vq->packed.desc_extra);
>  err_desc_extra:
>  	kfree(vq->packed.desc_state);
>  err_desc_state:
> @@ -2168,6 +2214,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>  					void (*callback)(struct virtqueue *),
>  					const char *name)
>  {
> +	struct virtio_driver *drv = drv_to_virtio(vdev->dev.driver);
>  	struct vring_virtqueue *vq;
>  
>  	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> @@ -2227,6 +2274,13 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>  	if (!vq->split.desc_extra)
>  		goto err_extra;
>  
> +	if (!drv->validate_used || force_used_validation) {
> +		vq->buflen = kmalloc_array(vring.num, sizeof(*vq->buflen),
> +					   GFP_KERNEL);
> +		if (!vq->buflen)
> +			goto err_buflen;
> +	}
> +
>  	/* Put everything in free lists. */
>  	vq->free_head = 0;
>  	memset(vq->split.desc_state, 0, vring.num *
> @@ -2237,6 +2291,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>  	spin_unlock(&vdev->vqs_list_lock);
>  	return &vq->vq;
>  
> +err_buflen:
> +	kfree(vq->split.desc_extra);
>  err_extra:
>  	kfree(vq->split.desc_state);
>  err_state:
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 0cd8685aeba4..79e4f6765e3a 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -153,6 +153,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev);
>   * @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
> + * @validate_used: whether the driver can validate used buffer length

So you set it to true to *not* have core validate used. Ugh.
I understand completely why it's like this: to have the default false
be safe. But let's call this out.

	@suppress_used_validation: set to not have core validate used length


>   * @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.
> @@ -169,6 +170,7 @@ struct virtio_driver {
>  	unsigned int feature_table_size;
>  	const unsigned int *feature_table_legacy;
>  	unsigned int feature_table_size_legacy;
> +	bool validate_used;
>  	int (*validate)(struct virtio_device *dev);
>  	int (*probe)(struct virtio_device *dev);
>  	void (*scan)(struct virtio_device *dev);
> -- 
> 2.25.1

Re: [PATCH V2 01/12] virtio-blk: validate num_queues during probe

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 02:52:16PM +0800, Jason Wang wrote:
> 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>
> Cc: Stefano Garzarella <sgarzare@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

I'd rather fail probe so we don't need to support that.

> ---
>  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 9b3bd083b411..9deff01a38cb 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -495,7 +495,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 one virtqueue */
> +	if (err || !num_vqs)
>  		num_vqs = 1;
>  
>  	num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
> -- 
> 2.25.1

Re: [PATCH V2 02/12] virtio: add doc for validate() method

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 02:52:17PM +0800, Jason Wang wrote:
> This patch adds doc for validate() method.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>

And maybe document __virtio_clear_bit : it says
"for use by transports" and now it's also legal in the
validate callback.

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

Re: [PATCH V2 03/12] virtio-console: switch to use .validate()

[Jason Wang]

On Wed, Oct 13, 2021 at 5:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 12, 2021 at 02:52:18PM +0800, Jason Wang wrote:
> > This patch switches to use validate() to filter out the features that
> > is not supported by the rproc.
>
> are not supported
>
> >
> > Cc: Amit Shah <amit@kernel.org>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
>
> Does this have anything to do with hardening?
>
> It seems cleaner to not negotiate features we do not use,
> but given we did this for many years ... should we bother
> at this point?

It looks cleaner to do all the validation in one places:

1) check unsupported feature for rproc
2) validate the max_nr_ports (as has been done in patch 4)

>
>
> > ---
> >  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! */
>
>
> This comment needs to be fixed now. And the format's wrong
> since you made it a multi-line comment.
> Should be
>         /*
>          * like
>          * this
>          */

Ok.

>
> > +             __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.
>
> This switches the order of tests around, so if an rproc device
> offers VIRTIO_CONSOLE_F_SIZE or VIRTIO_CONSOLE_F_MULTIPORT
> without get it will now try to work instead of failing.

Ok, so we can fail the validation in this case.

Thanks

>
> Which is maybe a worthy goal, but given rproc does not support
> virtio 1.0 it also risks trying to drive something completely
> unreasonable.
>
> Overall does not feel like hardening which is supposed to make
> things more strict, not less.
>
>
> > @@ -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
>

Re: [PATCH V2 06/12] virtio_pci: harden MSI-X interrupts

[Jason Wang]

On Wed, Oct 13, 2021 at 5:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 12, 2021 at 02:52:21PM +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.
>
> You mean until the driver is ready.

RIght, I will fix it.

Thanks

Re: [PATCH V2 09/12] virtio_ring: validate used buffer length

[Jason Wang]

On Wed, Oct 13, 2021 at 6:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 12, 2021 at 02:52:24PM +0800, Jason Wang wrote:
> > 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.
> >
> > Since some drivers have already done the validation by itself, this
>
> by themselves
>
> > patch tries to makes the core validation optional. For the driver that
> > doesn't want the validation, it can set the validate_used to be
> > true (which could be overridden by force_used_validation). To be more
> > efficient, a dedicate array is used for storing the validate used
> > length, this helps to eliminate the cache stress if validation is done
> > by the driver.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/virtio/virtio_ring.c | 56 ++++++++++++++++++++++++++++++++++++
> >  include/linux/virtio.h       |  2 ++
> >  2 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index d2ca0a7365f8..dee962bd8b04 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -14,6 +14,9 @@
> >  #include <linux/spinlock.h>
> >  #include <xen/xen.h>
> >
> > +static bool force_used_validation = false;
> > +module_param(force_used_validation, bool, 0444);
> > +
> >  #ifdef DEBUG
> >  /* For development, we want to crash whenever the ring is screwed. */
> >  #define BAD_RING(_vq, fmt, args...)                          \
> > @@ -182,6 +185,9 @@ struct vring_virtqueue {
> >               } packed;
> >       };
> >
> > +     /* Per-descriptor in buffer length */
> > +     u32 *buflen;
> > +
> >       /* How to notify other side. FIXME: commonalize hcalls! */
> >       bool (*notify)(struct virtqueue *vq);
> >
> > @@ -490,6 +496,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> >       unsigned int i, n, avail, descs_used, prev, err_idx;
> >       int head;
> >       bool indirect;
> > +     u32 buflen = 0;
> >
> >       START_USE(vq);
> >
> > @@ -571,6 +578,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. */
> > @@ -610,6 +618,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> >       else
> >               vq->split.desc_state[head].indir_desc = ctx;
> >
> > +     /* Store in buffer length if necessary */
> > +     if (vq->buflen)
> > +             vq->buflen[head] = buflen;
> > +
> >       /* Put entry in available array (but don't update avail->idx until they
> >        * do sync). */
> >       avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1);
> > @@ -784,6 +796,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 (vq->buflen && unlikely(*len > vq->buflen[i])) {
> > +             BAD_RING(vq, "used len %d is larger than in buflen %u\n",
> > +                     *len, vq->buflen[i]);
> > +             return NULL;
> > +     }
> >
> >       /* detach_buf_split clears data, so grab it now. */
> >       ret = vq->split.desc_state[i].data;
> > @@ -1062,6 +1079,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> >       unsigned int i, n, err_idx;
> >       u16 head, id;
> >       dma_addr_t addr;
> > +     u32 buflen = 0;
> >
> >       head = vq->packed.next_avail_idx;
> >       desc = alloc_indirect_packed(total_sg, gfp);
> > @@ -1089,6 +1107,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;
> >               }
> >       }
> >
> > @@ -1142,6 +1162,10 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> >       vq->packed.desc_state[id].indir_desc = desc;
> >       vq->packed.desc_state[id].last = id;
> >
> > +     /* Store in buffer length if necessary */
> > +     if (vq->buflen)
> > +             vq->buflen[id] = buflen;
> > +
> >       vq->num_added += 1;
> >
> >       pr_debug("Added buffer head %i to %p\n", head, vq);
> > @@ -1176,6 +1200,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;
> > +     u32 buflen = 0;
> >
> >       START_USE(vq);
> >
> > @@ -1250,6 +1275,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;
> >               }
> >       }
> >
> > @@ -1269,6 +1296,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> >       vq->packed.desc_state[id].indir_desc = ctx;
> >       vq->packed.desc_state[id].last = prev;
> >
> > +     /* Store in buffer length if necessary */
> > +     if (vq->buflen)
> > +             vq->buflen[id] = buflen;
> > +
> >       /*
> >        * A driver MUST NOT make the first descriptor in the list
> >        * available before all subsequent descriptors comprising
> > @@ -1455,6 +1486,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 (vq->buflen && unlikely(*len > vq->buflen[id])) {
> > +             BAD_RING(vq, "used len %d is larger than in buflen %u\n",
> > +                     *len, vq->buflen[id]);
> > +             return NULL;
> > +     }
> >
> >       /* detach_buf_packed clears data, so grab it now. */
> >       ret = vq->packed.desc_state[id].data;
> > @@ -1660,6 +1696,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >       struct vring_virtqueue *vq;
> >       struct vring_packed_desc *ring;
> >       struct vring_packed_desc_event *driver, *device;
> > +     struct virtio_driver *drv = drv_to_virtio(vdev->dev.driver);
> >       dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
> >       size_t ring_size_in_bytes, event_size_in_bytes;
> >
> > @@ -1749,6 +1786,13 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >       if (!vq->packed.desc_extra)
> >               goto err_desc_extra;
> >
> > +     if (!drv->validate_used || force_used_validation) {
> > +             vq->buflen = kmalloc_array(num, sizeof(*vq->buflen),
> > +                                        GFP_KERNEL);
> > +             if (!vq->buflen)
> > +                     goto err_buflen;
> > +     }
> > +
> >       /* No callback?  Tell other side not to bother us. */
> >       if (!callback) {
> >               vq->packed.event_flags_shadow = VRING_PACKED_EVENT_FLAG_DISABLE;
> > @@ -1761,6 +1805,8 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >       spin_unlock(&vdev->vqs_list_lock);
> >       return &vq->vq;
> >
> > +err_buflen:
> > +     kfree(vq->packed.desc_extra);
> >  err_desc_extra:
> >       kfree(vq->packed.desc_state);
> >  err_desc_state:
> > @@ -2168,6 +2214,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
> >                                       void (*callback)(struct virtqueue *),
> >                                       const char *name)
> >  {
> > +     struct virtio_driver *drv = drv_to_virtio(vdev->dev.driver);
> >       struct vring_virtqueue *vq;
> >
> >       if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> > @@ -2227,6 +2274,13 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
> >       if (!vq->split.desc_extra)
> >               goto err_extra;
> >
> > +     if (!drv->validate_used || force_used_validation) {
> > +             vq->buflen = kmalloc_array(vring.num, sizeof(*vq->buflen),
> > +                                        GFP_KERNEL);
> > +             if (!vq->buflen)
> > +                     goto err_buflen;
> > +     }
> > +
> >       /* Put everything in free lists. */
> >       vq->free_head = 0;
> >       memset(vq->split.desc_state, 0, vring.num *
> > @@ -2237,6 +2291,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
> >       spin_unlock(&vdev->vqs_list_lock);
> >       return &vq->vq;
> >
> > +err_buflen:
> > +     kfree(vq->split.desc_extra);
> >  err_extra:
> >       kfree(vq->split.desc_state);
> >  err_state:
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 0cd8685aeba4..79e4f6765e3a 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -153,6 +153,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev);
> >   * @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
> > + * @validate_used: whether the driver can validate used buffer length
>
> So you set it to true to *not* have core validate used. Ugh.
> I understand completely why it's like this: to have the default false
> be safe. But let's call this out.
>
>         @suppress_used_validation: set to not have core validate used length

Ok.

Thanks

>
>
> >   * @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.
> > @@ -169,6 +170,7 @@ struct virtio_driver {
> >       unsigned int feature_table_size;
> >       const unsigned int *feature_table_legacy;
> >       unsigned int feature_table_size_legacy;
> > +     bool validate_used;
> >       int (*validate)(struct virtio_device *dev);
> >       int (*probe)(struct virtio_device *dev);
> >       void (*scan)(struct virtio_device *dev);
> > --
> > 2.25.1
>

Re: [PATCH V2 01/12] virtio-blk: validate num_queues during probe

[Jason Wang]

On Wed, Oct 13, 2021 at 6:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 12, 2021 at 02:52:16PM +0800, Jason Wang wrote:
> > 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>
> > Cc: Stefano Garzarella <sgarzare@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> I'd rather fail probe so we don't need to support that.

I think we should be consistent among all virtio drivers.

E.g without this patch, we stick to 1 if virtio_create_feature() fail.
Do we need to fix that?

And we do something similar at least for the virtio-net and a lot of
other places.

        /* We need at least 2 queue's */
        if (err || max_queue_pairs < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
            max_queue_pairs > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
            !virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
                max_queue_pairs = 1;

Thanks

>
> > ---
> >  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 9b3bd083b411..9deff01a38cb 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -495,7 +495,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 one virtqueue */
> > +     if (err || !num_vqs)
> >               num_vqs = 1;
> >
> >       num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
> > --
> > 2.25.1
>

Re: [PATCH V2 02/12] virtio: add doc for validate() method

[Jason Wang]

On Wed, Oct 13, 2021 at 6:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 12, 2021 at 02:52:17PM +0800, Jason Wang wrote:
> > This patch adds doc for validate() method.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> And maybe document __virtio_clear_bit : it says
> "for use by transports" and now it's also legal in the
> validate callback.

Ok, let me do that in v3.

Thanks

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

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Jason Wang]

On Wed, Oct 13, 2021 at 5:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 12, 2021 at 02:52:22PM +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.
> >
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Paul E. McKenney <paulmck@kernel.org>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
> >  drivers/virtio/virtio_pci_common.h |  1 +
> >  2 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index 0b9523e6dd39..5ae6a2a4eb77 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -30,8 +30,16 @@ 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) {
> > +             /*
> > +              * The below synchronize() guarantees that any
> > +              * interrupt for this line arriving after
> > +              * synchronize_irq() has completed is guaranteed to see
> > +              * intx_soft_enabled == false.
> > +              */
> > +             WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> >               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 +51,16 @@ 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) {
> > +             disable_irq(vp_dev->pci_dev->irq);
> > +             /*
> > +              * The above disable_irq() provides TSO ordering and
> > +              * as such promotes the below store to store-release.
> > +              */
> > +             WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> > +             enable_irq(vp_dev->pci_dev->irq);
> >               return;
> > +     }
> >
> >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> >       struct virtio_pci_device *vp_dev = opaque;
> >       u8 isr;
> >
> > +     /* read intx_soft_enabled before read others */
> > +     if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> > +             return IRQ_NONE;
> > +
> >       /* 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);
>
> I don't see why we need this ordering guarantee here.
>
> synchronize_irq above makes sure no interrupt handler
> is in progress.

Yes.

> the handler itself thus does not need
> any specific order, it is ok if intx_soft_enabled is read
> after, not before the rest of it.

But the interrupt could be raised after synchronize_irq() which may
see a false of the intx_soft_enabled. In this case we still need the
make sure intx_soft_enbled to be read first instead of allowing other
operations to be done first, otherwise the intx_soft_enabled is
meaningless.

Thanks

>
> Just READ_ONCE should be enough, and we can drop the comment.
>
>
> > 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
>

Re: [PATCH V2 01/12] virtio-blk: validate num_queues during probe

[Michael S. Tsirkin]

On Thu, Oct 14, 2021 at 10:32:32AM +0800, Jason Wang wrote:
> On Wed, Oct 13, 2021 at 6:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 12, 2021 at 02:52:16PM +0800, Jason Wang wrote:
> > > 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>
> > > Cc: Stefano Garzarella <sgarzare@redhat.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > I'd rather fail probe so we don't need to support that.
> 
> I think we should be consistent among all virtio drivers.

Well we started being permissive. We can't change that
since that might break on some hosts. But given focus on
security being restrictive sounds better now.

> E.g without this patch, we stick to 1 if virtio_create_feature() fail.
> Do we need to fix that?

We can't easily, some hosts might be broken.

> And we do something similar at least for the virtio-net and a lot of
> other places.
> 
>         /* We need at least 2 queue's */
>         if (err || max_queue_pairs < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
>             max_queue_pairs > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
>             !virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>                 max_queue_pairs = 1;
> 
> Thanks
> 
> >
> > > ---
> > >  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 9b3bd083b411..9deff01a38cb 100644
> > > --- a/drivers/block/virtio_blk.c
> > > +++ b/drivers/block/virtio_blk.c
> > > @@ -495,7 +495,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 one virtqueue */
> > > +     if (err || !num_vqs)
> > >               num_vqs = 1;
> > >
> > >       num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
> > > --
> > > 2.25.1
> >

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Michael S. Tsirkin]

On Thu, Oct 14, 2021 at 10:35:48AM +0800, Jason Wang wrote:
> On Wed, Oct 13, 2021 at 5:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 12, 2021 at 02:52:22PM +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.
> > >
> > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
> > >  drivers/virtio/virtio_pci_common.h |  1 +
> > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > index 0b9523e6dd39..5ae6a2a4eb77 100644
> > > --- a/drivers/virtio/virtio_pci_common.c
> > > +++ b/drivers/virtio/virtio_pci_common.c
> > > @@ -30,8 +30,16 @@ 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) {
> > > +             /*
> > > +              * The below synchronize() guarantees that any
> > > +              * interrupt for this line arriving after
> > > +              * synchronize_irq() has completed is guaranteed to see
> > > +              * intx_soft_enabled == false.
> > > +              */
> > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> > >               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 +51,16 @@ 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) {
> > > +             disable_irq(vp_dev->pci_dev->irq);
> > > +             /*
> > > +              * The above disable_irq() provides TSO ordering and
> > > +              * as such promotes the below store to store-release.
> > > +              */
> > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> > > +             enable_irq(vp_dev->pci_dev->irq);
> > >               return;
> > > +     }
> > >
> > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> > >       struct virtio_pci_device *vp_dev = opaque;
> > >       u8 isr;
> > >
> > > +     /* read intx_soft_enabled before read others */
> > > +     if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> > > +             return IRQ_NONE;
> > > +
> > >       /* 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);
> >
> > I don't see why we need this ordering guarantee here.
> >
> > synchronize_irq above makes sure no interrupt handler
> > is in progress.
> 
> Yes.
> 
> > the handler itself thus does not need
> > any specific order, it is ok if intx_soft_enabled is read
> > after, not before the rest of it.
> 
> But the interrupt could be raised after synchronize_irq() which may
> see a false of the intx_soft_enabled.

You mean a "true" value right? false is what we are writing there.

Are you sure it can happen? I think that synchronize_irq makes the value
visible on all CPUs running the irq.

> In this case we still need the
> make sure intx_soft_enbled to be read first instead of allowing other
> operations to be done first, otherwise the intx_soft_enabled is
> meaningless.
> 
> Thanks

If intx_soft_enbled were not visible after synchronize_irq then
it does not matter in which order we read it wrt other values,
it still wouldn't work right.

> >
> > Just READ_ONCE should be enough, and we can drop the comment.
> >
> >
> > > 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
> >

Re: [PATCH V2 03/12] virtio-console: switch to use .validate()

[Michael S. Tsirkin]

On Thu, Oct 14, 2021 at 10:28:06AM +0800, Jason Wang wrote:
> On Wed, Oct 13, 2021 at 5:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 12, 2021 at 02:52:18PM +0800, Jason Wang wrote:
> > > This patch switches to use validate() to filter out the features that
> > > is not supported by the rproc.
> >
> > are not supported
> >
> > >
> > > Cc: Amit Shah <amit@kernel.org>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> >
> > Does this have anything to do with hardening?
> >
> > It seems cleaner to not negotiate features we do not use,
> > but given we did this for many years ... should we bother
> > at this point?
> 
> It looks cleaner to do all the validation in one places:
> 
> 1) check unsupported feature for rproc
> 2) validate the max_nr_ports (as has been done in patch 4)
> 
> >
> >
> > > ---
> > >  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! */
> >
> >
> > This comment needs to be fixed now. And the format's wrong
> > since you made it a multi-line comment.
> > Should be
> >         /*
> >          * like
> >          * this
> >          */
> 
> Ok.
> 
> >
> > > +             __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.
> >
> > This switches the order of tests around, so if an rproc device
> > offers VIRTIO_CONSOLE_F_SIZE or VIRTIO_CONSOLE_F_MULTIPORT
> > without get it will now try to work instead of failing.
> 
> Ok, so we can fail the validation in this case.

We can. But if we are going to, then it's easier to just fail probe.
Or if you want to try and work around this case,
that's also reasonable but pls document in the commit log.


> Thanks
> 
> >
> > Which is maybe a worthy goal, but given rproc does not support
> > virtio 1.0 it also risks trying to drive something completely
> > unreasonable.
> >
> > Overall does not feel like hardening which is supposed to make
> > things more strict, not less.
> >
> >
> > > @@ -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
> >

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Jason Wang]

On Thu, Oct 14, 2021 at 1:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Oct 14, 2021 at 10:35:48AM +0800, Jason Wang wrote:
> > On Wed, Oct 13, 2021 at 5:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Oct 12, 2021 at 02:52:22PM +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.
> > > >
> > > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
> > > >  drivers/virtio/virtio_pci_common.h |  1 +
> > > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > index 0b9523e6dd39..5ae6a2a4eb77 100644
> > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > @@ -30,8 +30,16 @@ 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) {
> > > > +             /*
> > > > +              * The below synchronize() guarantees that any
> > > > +              * interrupt for this line arriving after
> > > > +              * synchronize_irq() has completed is guaranteed to see
> > > > +              * intx_soft_enabled == false.
> > > > +              */
> > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> > > >               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 +51,16 @@ 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) {
> > > > +             disable_irq(vp_dev->pci_dev->irq);
> > > > +             /*
> > > > +              * The above disable_irq() provides TSO ordering and
> > > > +              * as such promotes the below store to store-release.
> > > > +              */
> > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> > > > +             enable_irq(vp_dev->pci_dev->irq);
> > > >               return;
> > > > +     }
> > > >
> > > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> > > >       struct virtio_pci_device *vp_dev = opaque;
> > > >       u8 isr;
> > > >
> > > > +     /* read intx_soft_enabled before read others */
> > > > +     if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> > > > +             return IRQ_NONE;
> > > > +
> > > >       /* 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);
> > >
> > > I don't see why we need this ordering guarantee here.
> > >
> > > synchronize_irq above makes sure no interrupt handler
> > > is in progress.
> >
> > Yes.
> >
> > > the handler itself thus does not need
> > > any specific order, it is ok if intx_soft_enabled is read
> > > after, not before the rest of it.
> >
> > But the interrupt could be raised after synchronize_irq() which may
> > see a false of the intx_soft_enabled.
>
> You mean a "true" value right? false is what we are writing there.

I meant that we want to not go for stuff like vq->callback after the
synchronize_irq() after setting intx_soft_enabled to false. Otherwise
we may get unexpected results like use after free. Host can craft ISR
in this case.

>
> Are you sure it can happen? I think that synchronize_irq makes the value
> visible on all CPUs running the irq.

Yes, so the false is visible by vp_interrupt(), we can't do the other
task before we check intx_soft_enabled.

>
> > In this case we still need the
> > make sure intx_soft_enbled to be read first instead of allowing other
> > operations to be done first, otherwise the intx_soft_enabled is
> > meaningless.
> >
> > Thanks
>
> If intx_soft_enbled were not visible after synchronize_irq then
> it does not matter in which order we read it wrt other values,
> it still wouldn't work right.

Yes.

Thanks

>
> > >
> > > Just READ_ONCE should be enough, and we can drop the comment.
> > >
> > >
> > > > 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
> > >
>

Re: [PATCH V2 01/12] virtio-blk: validate num_queues during probe

[Jason Wang]

On Thu, Oct 14, 2021 at 1:45 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Oct 14, 2021 at 10:32:32AM +0800, Jason Wang wrote:
> > On Wed, Oct 13, 2021 at 6:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Oct 12, 2021 at 02:52:16PM +0800, Jason Wang wrote:
> > > > 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>
> > > > Cc: Stefano Garzarella <sgarzare@redhat.com>
> > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >
> > > I'd rather fail probe so we don't need to support that.
> >
> > I think we should be consistent among all virtio drivers.
>
> Well we started being permissive. We can't change that
> since that might break on some hosts. But given focus on
> security being restrictive sounds better now.

Right.

>
> > E.g without this patch, we stick to 1 if virtio_create_feature() fail.
> > Do we need to fix that?
>
> We can't easily, some hosts might be broken.

Ok.

Thanks

>
> > And we do something similar at least for the virtio-net and a lot of
> > other places.
> >
> >         /* We need at least 2 queue's */
> >         if (err || max_queue_pairs < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> >             max_queue_pairs > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
> >             !virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> >                 max_queue_pairs = 1;
> >
> > Thanks
> >
> > >
> > > > ---
> > > >  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 9b3bd083b411..9deff01a38cb 100644
> > > > --- a/drivers/block/virtio_blk.c
> > > > +++ b/drivers/block/virtio_blk.c
> > > > @@ -495,7 +495,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 one virtqueue */
> > > > +     if (err || !num_vqs)
> > > >               num_vqs = 1;
> > > >
> > > >       num_vqs = min_t(unsigned int, nr_cpu_ids, num_vqs);
> > > > --
> > > > 2.25.1
> > >
>

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Michael S. Tsirkin]

On Thu, Oct 14, 2021 at 02:20:17PM +0800, Jason Wang wrote:
> On Thu, Oct 14, 2021 at 1:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Oct 14, 2021 at 10:35:48AM +0800, Jason Wang wrote:
> > > On Wed, Oct 13, 2021 at 5:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Oct 12, 2021 at 02:52:22PM +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.
> > > > >
> > > > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > ---
> > > > >  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
> > > > >  drivers/virtio/virtio_pci_common.h |  1 +
> > > > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > index 0b9523e6dd39..5ae6a2a4eb77 100644
> > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > @@ -30,8 +30,16 @@ 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) {
> > > > > +             /*
> > > > > +              * The below synchronize() guarantees that any
> > > > > +              * interrupt for this line arriving after
> > > > > +              * synchronize_irq() has completed is guaranteed to see
> > > > > +              * intx_soft_enabled == false.
> > > > > +              */
> > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> > > > >               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 +51,16 @@ 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) {
> > > > > +             disable_irq(vp_dev->pci_dev->irq);
> > > > > +             /*
> > > > > +              * The above disable_irq() provides TSO ordering and
> > > > > +              * as such promotes the below store to store-release.
> > > > > +              */
> > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> > > > > +             enable_irq(vp_dev->pci_dev->irq);
> > > > >               return;
> > > > > +     }
> > > > >
> > > > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> > > > >       struct virtio_pci_device *vp_dev = opaque;
> > > > >       u8 isr;
> > > > >
> > > > > +     /* read intx_soft_enabled before read others */
> > > > > +     if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> > > > > +             return IRQ_NONE;
> > > > > +
> > > > >       /* 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);
> > > >
> > > > I don't see why we need this ordering guarantee here.
> > > >
> > > > synchronize_irq above makes sure no interrupt handler
> > > > is in progress.
> > >
> > > Yes.
> > >
> > > > the handler itself thus does not need
> > > > any specific order, it is ok if intx_soft_enabled is read
> > > > after, not before the rest of it.
> > >
> > > But the interrupt could be raised after synchronize_irq() which may
> > > see a false of the intx_soft_enabled.
> >
> > You mean a "true" value right? false is what we are writing there.
> 
> I meant that we want to not go for stuff like vq->callback after the
> synchronize_irq() after setting intx_soft_enabled to false. Otherwise
> we may get unexpected results like use after free. Host can craft ISR
> in this case.
> >
> > Are you sure it can happen? I think that synchronize_irq makes the value
> > visible on all CPUs running the irq.
> 
> Yes, so the false is visible by vp_interrupt(), we can't do the other
> task before we check intx_soft_enabled.

But the order does not matter. synchronize_irq will make sure
everything is visible.

> >
> > > In this case we still need the
> > > make sure intx_soft_enbled to be read first instead of allowing other
> > > operations to be done first, otherwise the intx_soft_enabled is
> > > meaningless.
> > >
> > > Thanks
> >
> > If intx_soft_enbled were not visible after synchronize_irq then
> > it does not matter in which order we read it wrt other values,
> > it still wouldn't work right.
> 
> Yes.
> 
> Thanks


We are agreed then? No need for a barrier here, READ_ONCE is enough?

> >
> > > >
> > > > Just READ_ONCE should be enough, and we can drop the comment.
> > > >
> > > >
> > > > > 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
> > > >
> >

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Jason Wang]

On Thu, Oct 14, 2021 at 2:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Oct 14, 2021 at 02:20:17PM +0800, Jason Wang wrote:
> > On Thu, Oct 14, 2021 at 1:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Oct 14, 2021 at 10:35:48AM +0800, Jason Wang wrote:
> > > > On Wed, Oct 13, 2021 at 5:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Tue, Oct 12, 2021 at 02:52:22PM +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.
> > > > > >
> > > > > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > ---
> > > > > >  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
> > > > > >  drivers/virtio/virtio_pci_common.h |  1 +
> > > > > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > index 0b9523e6dd39..5ae6a2a4eb77 100644
> > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > @@ -30,8 +30,16 @@ 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) {
> > > > > > +             /*
> > > > > > +              * The below synchronize() guarantees that any
> > > > > > +              * interrupt for this line arriving after
> > > > > > +              * synchronize_irq() has completed is guaranteed to see
> > > > > > +              * intx_soft_enabled == false.
> > > > > > +              */
> > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> > > > > >               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 +51,16 @@ 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) {
> > > > > > +             disable_irq(vp_dev->pci_dev->irq);
> > > > > > +             /*
> > > > > > +              * The above disable_irq() provides TSO ordering and
> > > > > > +              * as such promotes the below store to store-release.
> > > > > > +              */
> > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> > > > > > +             enable_irq(vp_dev->pci_dev->irq);
> > > > > >               return;
> > > > > > +     }
> > > > > >
> > > > > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> > > > > >       struct virtio_pci_device *vp_dev = opaque;
> > > > > >       u8 isr;
> > > > > >
> > > > > > +     /* read intx_soft_enabled before read others */
> > > > > > +     if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> > > > > > +             return IRQ_NONE;
> > > > > > +
> > > > > >       /* 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);
> > > > >
> > > > > I don't see why we need this ordering guarantee here.
> > > > >
> > > > > synchronize_irq above makes sure no interrupt handler
> > > > > is in progress.
> > > >
> > > > Yes.
> > > >
> > > > > the handler itself thus does not need
> > > > > any specific order, it is ok if intx_soft_enabled is read
> > > > > after, not before the rest of it.
> > > >
> > > > But the interrupt could be raised after synchronize_irq() which may
> > > > see a false of the intx_soft_enabled.
> > >
> > > You mean a "true" value right? false is what we are writing there.
> >
> > I meant that we want to not go for stuff like vq->callback after the
> > synchronize_irq() after setting intx_soft_enabled to false. Otherwise
> > we may get unexpected results like use after free. Host can craft ISR
> > in this case.
> > >
> > > Are you sure it can happen? I think that synchronize_irq makes the value
> > > visible on all CPUs running the irq.
> >
> > Yes, so the false is visible by vp_interrupt(), we can't do the other
> > task before we check intx_soft_enabled.
>
> But the order does not matter. synchronize_irq will make sure
> everything is visible.

Not the thing that happens after synchronize_irq().

E.g for remove_vq_common():

static void remove_vq_common(struct virtnet_info *vi)
{
        vi->vdev->config->reset(vi->vdev);

        /* Free unused buffers in both send and recv, if any. */
        free_unused_bufs(vi);

        free_receive_bufs(vi);

        free_receive_page_frags(vi);

        virtnet_del_vqs(vi);
}

The interrupt could be raised by the device after .reset().

Thanks

>
> > >
> > > > In this case we still need the
> > > > make sure intx_soft_enbled to be read first instead of allowing other
> > > > operations to be done first, otherwise the intx_soft_enabled is
> > > > meaningless.
> > > >
> > > > Thanks
> > >
> > > If intx_soft_enbled were not visible after synchronize_irq then
> > > it does not matter in which order we read it wrt other values,
> > > it still wouldn't work right.
> >
> > Yes.
> >
> > Thanks
>
>
> We are agreed then? No need for a barrier here, READ_ONCE is enough?
>
> > >
> > > > >
> > > > > Just READ_ONCE should be enough, and we can drop the comment.
> > > > >
> > > > >
> > > > > > 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
> > > > >
> > >
>

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Michael S. Tsirkin]

On Thu, Oct 14, 2021 at 02:32:19PM +0800, Jason Wang wrote:
> On Thu, Oct 14, 2021 at 2:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Oct 14, 2021 at 02:20:17PM +0800, Jason Wang wrote:
> > > On Thu, Oct 14, 2021 at 1:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 14, 2021 at 10:35:48AM +0800, Jason Wang wrote:
> > > > > On Wed, Oct 13, 2021 at 5:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, Oct 12, 2021 at 02:52:22PM +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.
> > > > > > >
> > > > > > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > ---
> > > > > > >  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
> > > > > > >  drivers/virtio/virtio_pci_common.h |  1 +
> > > > > > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > > index 0b9523e6dd39..5ae6a2a4eb77 100644
> > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > @@ -30,8 +30,16 @@ 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) {
> > > > > > > +             /*
> > > > > > > +              * The below synchronize() guarantees that any
> > > > > > > +              * interrupt for this line arriving after
> > > > > > > +              * synchronize_irq() has completed is guaranteed to see
> > > > > > > +              * intx_soft_enabled == false.
> > > > > > > +              */
> > > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> > > > > > >               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 +51,16 @@ 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) {
> > > > > > > +             disable_irq(vp_dev->pci_dev->irq);
> > > > > > > +             /*
> > > > > > > +              * The above disable_irq() provides TSO ordering and
> > > > > > > +              * as such promotes the below store to store-release.
> > > > > > > +              */
> > > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> > > > > > > +             enable_irq(vp_dev->pci_dev->irq);
> > > > > > >               return;
> > > > > > > +     }
> > > > > > >
> > > > > > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > > @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> > > > > > >       struct virtio_pci_device *vp_dev = opaque;
> > > > > > >       u8 isr;
> > > > > > >
> > > > > > > +     /* read intx_soft_enabled before read others */
> > > > > > > +     if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> > > > > > > +             return IRQ_NONE;
> > > > > > > +
> > > > > > >       /* 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);
> > > > > >
> > > > > > I don't see why we need this ordering guarantee here.
> > > > > >
> > > > > > synchronize_irq above makes sure no interrupt handler
> > > > > > is in progress.
> > > > >
> > > > > Yes.
> > > > >
> > > > > > the handler itself thus does not need
> > > > > > any specific order, it is ok if intx_soft_enabled is read
> > > > > > after, not before the rest of it.
> > > > >
> > > > > But the interrupt could be raised after synchronize_irq() which may
> > > > > see a false of the intx_soft_enabled.
> > > >
> > > > You mean a "true" value right? false is what we are writing there.
> > >
> > > I meant that we want to not go for stuff like vq->callback after the
> > > synchronize_irq() after setting intx_soft_enabled to false. Otherwise
> > > we may get unexpected results like use after free. Host can craft ISR
> > > in this case.
> > > >
> > > > Are you sure it can happen? I think that synchronize_irq makes the value
> > > > visible on all CPUs running the irq.
> > >
> > > Yes, so the false is visible by vp_interrupt(), we can't do the other
> > > task before we check intx_soft_enabled.
> >
> > But the order does not matter. synchronize_irq will make sure
> > everything is visible.
> 
> Not the thing that happens after synchronize_irq().
> 
> E.g for remove_vq_common():
> 
> static void remove_vq_common(struct virtnet_info *vi)
> {
>         vi->vdev->config->reset(vi->vdev);
> 
>         /* Free unused buffers in both send and recv, if any. */
>         free_unused_bufs(vi);
> 
>         free_receive_bufs(vi);
> 
>         free_receive_page_frags(vi);
> 
>         virtnet_del_vqs(vi);
> }
> 
> The interrupt could be raised by the device after .reset().
> 
> Thanks

That's why your patches set intx_soft_enabled to false within reset.
Then you sync so all other CPUs see the false value.
Then it's ok to proceed with reset.
What does the interrupt handler *do* with the value
does not matter as long as it sees that it is false.

OTOH if you are really worried about spectre type speculative attacks,
that is a different matter, and would force us to stick expensive
barriers around hardware accessible buffers just like we have in
copy_XXX_user. I am not sure this is in scope for TDX, and
certainly out of scope for regular driver ardening.
If yes worth hiding that behind a kernel option.


> >
> > > >
> > > > > In this case we still need the
> > > > > make sure intx_soft_enbled to be read first instead of allowing other
> > > > > operations to be done first, otherwise the intx_soft_enabled is
> > > > > meaningless.
> > > > >
> > > > > Thanks
> > > >
> > > > If intx_soft_enbled were not visible after synchronize_irq then
> > > > it does not matter in which order we read it wrt other values,
> > > > it still wouldn't work right.
> > >
> > > Yes.
> > >
> > > Thanks
> >
> >
> > We are agreed then? No need for a barrier here, READ_ONCE is enough?
> >
> > > >
> > > > > >
> > > > > > Just READ_ONCE should be enough, and we can drop the comment.
> > > > > >
> > > > > >
> > > > > > > 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
> > > > > >
> > > >
> >

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Jason Wang]

On Thu, Oct 14, 2021 at 3:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Oct 14, 2021 at 02:32:19PM +0800, Jason Wang wrote:
> > On Thu, Oct 14, 2021 at 2:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Oct 14, 2021 at 02:20:17PM +0800, Jason Wang wrote:
> > > > On Thu, Oct 14, 2021 at 1:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 14, 2021 at 10:35:48AM +0800, Jason Wang wrote:
> > > > > > On Wed, Oct 13, 2021 at 5:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > On Tue, Oct 12, 2021 at 02:52:22PM +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.
> > > > > > > >
> > > > > > > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > > > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > > > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > ---
> > > > > > > >  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
> > > > > > > >  drivers/virtio/virtio_pci_common.h |  1 +
> > > > > > > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > > > index 0b9523e6dd39..5ae6a2a4eb77 100644
> > > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > > @@ -30,8 +30,16 @@ 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) {
> > > > > > > > +             /*
> > > > > > > > +              * The below synchronize() guarantees that any
> > > > > > > > +              * interrupt for this line arriving after
> > > > > > > > +              * synchronize_irq() has completed is guaranteed to see
> > > > > > > > +              * intx_soft_enabled == false.
> > > > > > > > +              */
> > > > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> > > > > > > >               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 +51,16 @@ 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) {
> > > > > > > > +             disable_irq(vp_dev->pci_dev->irq);
> > > > > > > > +             /*
> > > > > > > > +              * The above disable_irq() provides TSO ordering and
> > > > > > > > +              * as such promotes the below store to store-release.
> > > > > > > > +              */
> > > > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> > > > > > > > +             enable_irq(vp_dev->pci_dev->irq);
> > > > > > > >               return;
> > > > > > > > +     }
> > > > > > > >
> > > > > > > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > > >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > > > @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> > > > > > > >       struct virtio_pci_device *vp_dev = opaque;
> > > > > > > >       u8 isr;
> > > > > > > >
> > > > > > > > +     /* read intx_soft_enabled before read others */
> > > > > > > > +     if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> > > > > > > > +             return IRQ_NONE;
> > > > > > > > +
> > > > > > > >       /* 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);
> > > > > > >
> > > > > > > I don't see why we need this ordering guarantee here.
> > > > > > >
> > > > > > > synchronize_irq above makes sure no interrupt handler
> > > > > > > is in progress.
> > > > > >
> > > > > > Yes.
> > > > > >
> > > > > > > the handler itself thus does not need
> > > > > > > any specific order, it is ok if intx_soft_enabled is read
> > > > > > > after, not before the rest of it.
> > > > > >
> > > > > > But the interrupt could be raised after synchronize_irq() which may
> > > > > > see a false of the intx_soft_enabled.
> > > > >
> > > > > You mean a "true" value right? false is what we are writing there.
> > > >
> > > > I meant that we want to not go for stuff like vq->callback after the
> > > > synchronize_irq() after setting intx_soft_enabled to false. Otherwise
> > > > we may get unexpected results like use after free. Host can craft ISR
> > > > in this case.
> > > > >
> > > > > Are you sure it can happen? I think that synchronize_irq makes the value
> > > > > visible on all CPUs running the irq.
> > > >
> > > > Yes, so the false is visible by vp_interrupt(), we can't do the other
> > > > task before we check intx_soft_enabled.
> > >
> > > But the order does not matter. synchronize_irq will make sure
> > > everything is visible.
> >
> > Not the thing that happens after synchronize_irq().
> >
> > E.g for remove_vq_common():
> >
> > static void remove_vq_common(struct virtnet_info *vi)
> > {
> >         vi->vdev->config->reset(vi->vdev);
> >
> >         /* Free unused buffers in both send and recv, if any. */
> >         free_unused_bufs(vi);
> >
> >         free_receive_bufs(vi);
> >
> >         free_receive_page_frags(vi);
> >
> >         virtnet_del_vqs(vi);
> > }
> >
> > The interrupt could be raised by the device after .reset().
> >
> > Thanks
>
> That's why your patches set intx_soft_enabled to false within reset.
> Then you sync so all other CPUs see the false value.
> Then it's ok to proceed with reset.
> What does the interrupt handler *do* with the value
> does not matter as long as it sees that it is false.

I'm not sure I get here, if we allow the interrupt handler to access
the vq before checking intx_soft_enabled, won't there be a
use-after-free?

>
> OTOH if you are really worried about spectre type speculative attacks,
> that is a different matter, and would force us to stick expensive
> barriers around hardware accessible buffers just like we have in
> copy_XXX_user. I am not sure this is in scope for TDX, and
> certainly out of scope for regular driver ardening.
> If yes worth hiding that behind a kernel option.

Right.

Thanks

>
>
> > >
> > > > >
> > > > > > In this case we still need the
> > > > > > make sure intx_soft_enbled to be read first instead of allowing other
> > > > > > operations to be done first, otherwise the intx_soft_enabled is
> > > > > > meaningless.
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > If intx_soft_enbled were not visible after synchronize_irq then
> > > > > it does not matter in which order we read it wrt other values,
> > > > > it still wouldn't work right.
> > > >
> > > > Yes.
> > > >
> > > > Thanks
> > >
> > >
> > > We are agreed then? No need for a barrier here, READ_ONCE is enough?
> > >
> > > > >
> > > > > > >
> > > > > > > Just READ_ONCE should be enough, and we can drop the comment.
> > > > > > >
> > > > > > >
> > > > > > > > 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
> > > > > > >
> > > > >
> > >
>

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Michael S. Tsirkin]

On Thu, Oct 14, 2021 at 03:12:54PM +0800, Jason Wang wrote:
> On Thu, Oct 14, 2021 at 3:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Oct 14, 2021 at 02:32:19PM +0800, Jason Wang wrote:
> > > On Thu, Oct 14, 2021 at 2:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 14, 2021 at 02:20:17PM +0800, Jason Wang wrote:
> > > > > On Thu, Oct 14, 2021 at 1:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 14, 2021 at 10:35:48AM +0800, Jason Wang wrote:
> > > > > > > On Wed, Oct 13, 2021 at 5:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Oct 12, 2021 at 02:52:22PM +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.
> > > > > > > > >
> > > > > > > > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > > > > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > > > > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
> > > > > > > > >  drivers/virtio/virtio_pci_common.h |  1 +
> > > > > > > > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > > > > index 0b9523e6dd39..5ae6a2a4eb77 100644
> > > > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > > > @@ -30,8 +30,16 @@ 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) {
> > > > > > > > > +             /*
> > > > > > > > > +              * The below synchronize() guarantees that any
> > > > > > > > > +              * interrupt for this line arriving after
> > > > > > > > > +              * synchronize_irq() has completed is guaranteed to see
> > > > > > > > > +              * intx_soft_enabled == false.
> > > > > > > > > +              */
> > > > > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> > > > > > > > >               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 +51,16 @@ 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) {
> > > > > > > > > +             disable_irq(vp_dev->pci_dev->irq);
> > > > > > > > > +             /*
> > > > > > > > > +              * The above disable_irq() provides TSO ordering and
> > > > > > > > > +              * as such promotes the below store to store-release.
> > > > > > > > > +              */
> > > > > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> > > > > > > > > +             enable_irq(vp_dev->pci_dev->irq);
> > > > > > > > >               return;
> > > > > > > > > +     }
> > > > > > > > >
> > > > > > > > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > > > >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > > > > @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> > > > > > > > >       struct virtio_pci_device *vp_dev = opaque;
> > > > > > > > >       u8 isr;
> > > > > > > > >
> > > > > > > > > +     /* read intx_soft_enabled before read others */
> > > > > > > > > +     if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> > > > > > > > > +             return IRQ_NONE;
> > > > > > > > > +
> > > > > > > > >       /* 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);
> > > > > > > >
> > > > > > > > I don't see why we need this ordering guarantee here.
> > > > > > > >
> > > > > > > > synchronize_irq above makes sure no interrupt handler
> > > > > > > > is in progress.
> > > > > > >
> > > > > > > Yes.
> > > > > > >
> > > > > > > > the handler itself thus does not need
> > > > > > > > any specific order, it is ok if intx_soft_enabled is read
> > > > > > > > after, not before the rest of it.
> > > > > > >
> > > > > > > But the interrupt could be raised after synchronize_irq() which may
> > > > > > > see a false of the intx_soft_enabled.
> > > > > >
> > > > > > You mean a "true" value right? false is what we are writing there.
> > > > >
> > > > > I meant that we want to not go for stuff like vq->callback after the
> > > > > synchronize_irq() after setting intx_soft_enabled to false. Otherwise
> > > > > we may get unexpected results like use after free. Host can craft ISR
> > > > > in this case.
> > > > > >
> > > > > > Are you sure it can happen? I think that synchronize_irq makes the value
> > > > > > visible on all CPUs running the irq.
> > > > >
> > > > > Yes, so the false is visible by vp_interrupt(), we can't do the other
> > > > > task before we check intx_soft_enabled.
> > > >
> > > > But the order does not matter. synchronize_irq will make sure
> > > > everything is visible.
> > >
> > > Not the thing that happens after synchronize_irq().
> > >
> > > E.g for remove_vq_common():
> > >
> > > static void remove_vq_common(struct virtnet_info *vi)
> > > {
> > >         vi->vdev->config->reset(vi->vdev);
> > >
> > >         /* Free unused buffers in both send and recv, if any. */
> > >         free_unused_bufs(vi);
> > >
> > >         free_receive_bufs(vi);
> > >
> > >         free_receive_page_frags(vi);
> > >
> > >         virtnet_del_vqs(vi);
> > > }
> > >
> > > The interrupt could be raised by the device after .reset().
> > >
> > > Thanks
> >
> > That's why your patches set intx_soft_enabled to false within reset.
> > Then you sync so all other CPUs see the false value.
> > Then it's ok to proceed with reset.
> > What does the interrupt handler *do* with the value
> > does not matter as long as it sees that it is false.
> 
> I'm not sure I get here, if we allow the interrupt handler to access
> the vq before checking intx_soft_enabled, won't there be a
> use-after-free?

It's a speculative access, not an architectural one.

> >
> > OTOH if you are really worried about spectre type speculative attacks,
> > that is a different matter, and would force us to stick expensive
> > barriers around hardware accessible buffers just like we have in
> > copy_XXX_user. I am not sure this is in scope for TDX, and
> > certainly out of scope for regular driver ardening.
> > If yes worth hiding that behind a kernel option.
> 
> Right.
> 
> Thanks
> 
> >
> >
> > > >
> > > > > >
> > > > > > > In this case we still need the
> > > > > > > make sure intx_soft_enbled to be read first instead of allowing other
> > > > > > > operations to be done first, otherwise the intx_soft_enabled is
> > > > > > > meaningless.
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > > If intx_soft_enbled were not visible after synchronize_irq then
> > > > > > it does not matter in which order we read it wrt other values,
> > > > > > it still wouldn't work right.
> > > > >
> > > > > Yes.
> > > > >
> > > > > Thanks
> > > >
> > > >
> > > > We are agreed then? No need for a barrier here, READ_ONCE is enough?
> > > >
> > > > > >
> > > > > > > >
> > > > > > > > Just READ_ONCE should be enough, and we can drop the comment.
> > > > > > > >
> > > > > > > >
> > > > > > > > > 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
> > > > > > > >
> > > > > >
> > > >
> >

Re: [PATCH V2 07/12] virtio-pci: harden INTX interrupts

[Jason Wang]

On Thu, Oct 14, 2021 at 5:25 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Oct 14, 2021 at 03:12:54PM +0800, Jason Wang wrote:
> > On Thu, Oct 14, 2021 at 3:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Oct 14, 2021 at 02:32:19PM +0800, Jason Wang wrote:
> > > > On Thu, Oct 14, 2021 at 2:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 14, 2021 at 02:20:17PM +0800, Jason Wang wrote:
> > > > > > On Thu, Oct 14, 2021 at 1:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 14, 2021 at 10:35:48AM +0800, Jason Wang wrote:
> > > > > > > > On Wed, Oct 13, 2021 at 5:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Oct 12, 2021 at 02:52:22PM +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.
> > > > > > > > > >
> > > > > > > > > > Cc: Boqun Feng <boqun.feng@gmail.com>
> > > > > > > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > > > > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > > > > > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++++++--
> > > > > > > > > >  drivers/virtio/virtio_pci_common.h |  1 +
> > > > > > > > > >  2 files changed, 23 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > > > > > index 0b9523e6dd39..5ae6a2a4eb77 100644
> > > > > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > > > > @@ -30,8 +30,16 @@ 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) {
> > > > > > > > > > +             /*
> > > > > > > > > > +              * The below synchronize() guarantees that any
> > > > > > > > > > +              * interrupt for this line arriving after
> > > > > > > > > > +              * synchronize_irq() has completed is guaranteed to see
> > > > > > > > > > +              * intx_soft_enabled == false.
> > > > > > > > > > +              */
> > > > > > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, false);
> > > > > > > > > >               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 +51,16 @@ 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) {
> > > > > > > > > > +             disable_irq(vp_dev->pci_dev->irq);
> > > > > > > > > > +             /*
> > > > > > > > > > +              * The above disable_irq() provides TSO ordering and
> > > > > > > > > > +              * as such promotes the below store to store-release.
> > > > > > > > > > +              */
> > > > > > > > > > +             WRITE_ONCE(vp_dev->intx_soft_enabled, true);
> > > > > > > > > > +             enable_irq(vp_dev->pci_dev->irq);
> > > > > > > > > >               return;
> > > > > > > > > > +     }
> > > > > > > > > >
> > > > > > > > > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > > > > >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > > > > > @@ -97,6 +113,10 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> > > > > > > > > >       struct virtio_pci_device *vp_dev = opaque;
> > > > > > > > > >       u8 isr;
> > > > > > > > > >
> > > > > > > > > > +     /* read intx_soft_enabled before read others */
> > > > > > > > > > +     if (!smp_load_acquire(&vp_dev->intx_soft_enabled))
> > > > > > > > > > +             return IRQ_NONE;
> > > > > > > > > > +
> > > > > > > > > >       /* 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);
> > > > > > > > >
> > > > > > > > > I don't see why we need this ordering guarantee here.
> > > > > > > > >
> > > > > > > > > synchronize_irq above makes sure no interrupt handler
> > > > > > > > > is in progress.
> > > > > > > >
> > > > > > > > Yes.
> > > > > > > >
> > > > > > > > > the handler itself thus does not need
> > > > > > > > > any specific order, it is ok if intx_soft_enabled is read
> > > > > > > > > after, not before the rest of it.
> > > > > > > >
> > > > > > > > But the interrupt could be raised after synchronize_irq() which may
> > > > > > > > see a false of the intx_soft_enabled.
> > > > > > >
> > > > > > > You mean a "true" value right? false is what we are writing there.
> > > > > >
> > > > > > I meant that we want to not go for stuff like vq->callback after the
> > > > > > synchronize_irq() after setting intx_soft_enabled to false. Otherwise
> > > > > > we may get unexpected results like use after free. Host can craft ISR
> > > > > > in this case.
> > > > > > >
> > > > > > > Are you sure it can happen? I think that synchronize_irq makes the value
> > > > > > > visible on all CPUs running the irq.
> > > > > >
> > > > > > Yes, so the false is visible by vp_interrupt(), we can't do the other
> > > > > > task before we check intx_soft_enabled.
> > > > >
> > > > > But the order does not matter. synchronize_irq will make sure
> > > > > everything is visible.
> > > >
> > > > Not the thing that happens after synchronize_irq().
> > > >
> > > > E.g for remove_vq_common():
> > > >
> > > > static void remove_vq_common(struct virtnet_info *vi)
> > > > {
> > > >         vi->vdev->config->reset(vi->vdev);
> > > >
> > > >         /* Free unused buffers in both send and recv, if any. */
> > > >         free_unused_bufs(vi);
> > > >
> > > >         free_receive_bufs(vi);
> > > >
> > > >         free_receive_page_frags(vi);
> > > >
> > > >         virtnet_del_vqs(vi);
> > > > }
> > > >
> > > > The interrupt could be raised by the device after .reset().
> > > >
> > > > Thanks
> > >
> > > That's why your patches set intx_soft_enabled to false within reset.
> > > Then you sync so all other CPUs see the false value.
> > > Then it's ok to proceed with reset.
> > > What does the interrupt handler *do* with the value
> > > does not matter as long as it sees that it is false.
> >
> > I'm not sure I get here, if we allow the interrupt handler to access
> > the vq before checking intx_soft_enabled, won't there be a
> > use-after-free?
>
> It's a speculative access, not an architectural one.

Right. I will use READ_ONCE() in the next version.

Thanks

>
> > >
> > > OTOH if you are really worried about spectre type speculative attacks,
> > > that is a different matter, and would force us to stick expensive
> > > barriers around hardware accessible buffers just like we have in
> > > copy_XXX_user. I am not sure this is in scope for TDX, and
> > > certainly out of scope for regular driver ardening.
> > > If yes worth hiding that behind a kernel option.
> >
> > Right.
> >
> > Thanks
> >
> > >
> > >
> > > > >
> > > > > > >
> > > > > > > > In this case we still need the
> > > > > > > > make sure intx_soft_enbled to be read first instead of allowing other
> > > > > > > > operations to be done first, otherwise the intx_soft_enabled is
> > > > > > > > meaningless.
> > > > > > > >
> > > > > > > > Thanks
> > > > > > >
> > > > > > > If intx_soft_enbled were not visible after synchronize_irq then
> > > > > > > it does not matter in which order we read it wrt other values,
> > > > > > > it still wouldn't work right.
> > > > > >
> > > > > > Yes.
> > > > > >
> > > > > > Thanks
> > > > >
> > > > >
> > > > > We are agreed then? No need for a barrier here, READ_ONCE is enough?
> > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > Just READ_ONCE should be enough, and we can drop the comment.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > 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
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>

Re: [PATCH V2 06/12] virtio_pci: harden MSI-X interrupts

[Dongli Zhang]

Hi Jason,

On 10/11/21 11:52 PM, 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

About "unexpected results", while you mentioned below in v1 ...

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

.. and you also mentioned to avoid the panic (due to untrusted device) in as
many scenarios as possible.


Here is my understanding.

The reason we do not trust hypervisor is to protect (1) data/code privacy for
most of times and sometimes (2) program execution integrity.

The bad thing is: the hypervisor is able to panic/destroy the VM in the worst case.

It is reasonable to re-configure/recover if we assume there is BUG at
hypervisor/device side. That is, the hypervisor/device is buggy, but not malicious.

However, how about to just detect and report the hypervisor/device is malicious
and shutdown/panic the VM immediately? If we have detected the malicious
hypervisor, we should avoid running VMs on the malicious hypervisor further. At
least how about to print error message to reminder users that the hypervisor is
malicious?


About "unexpected results", it should not be hang/panic. I suggest ...

Assuming SEV/TDX is involved, the hypervisor should never be able to derive the
VM privacy (at least secure memory data) by providing malicious configuration,
e.g., num_queues=0. If we detect hypervisor is malicious, the VM is
panic/shutdown immediately. At least how about to print error message to
reminder users.


BTW, while I always do care about the loss of interrupt issue, the malicious
device is able to hang a VM by dropping a single interrupt on purpose for
virtio-scsi :)


Thank you very much!

Dongli Zhang

> 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.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Signed-off-by: Jason Wang <jasowang@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,
> 

Re: [PATCH V2 06/12] virtio_pci: harden MSI-X interrupts

[Michael S. Tsirkin]

On Fri, Oct 15, 2021 at 05:09:38AM -0700, Dongli Zhang wrote:
> Hi Jason,
> 
> On 10/11/21 11:52 PM, 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
> 
> About "unexpected results", while you mentioned below in v1 ...
> 
> "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."
> 
> .. and you also mentioned to avoid the panic (due to untrusted device) in as
> many scenarios as possible.
> 
> 
> Here is my understanding.
> 
> The reason we do not trust hypervisor is to protect (1) data/code privacy for
> most of times and sometimes (2) program execution integrity.
> 
> The bad thing is: the hypervisor is able to panic/destroy the VM in the worst case.
> 
> It is reasonable to re-configure/recover if we assume there is BUG at
> hypervisor/device side. That is, the hypervisor/device is buggy, but not malicious.
> 
> However, how about to just detect and report the hypervisor/device is malicious
> and shutdown/panic the VM immediately? If we have detected the malicious
> hypervisor, we should avoid running VMs on the malicious hypervisor further. At
> least how about to print error message to reminder users that the hypervisor is
> malicious?
> 
> 
> About "unexpected results", it should not be hang/panic. I suggest ...
> 
> Assuming SEV/TDX is involved, the hypervisor should never be able to derive the
> VM privacy (at least secure memory data) by providing malicious configuration,
> e.g., num_queues=0. If we detect hypervisor is malicious, the VM is
> panic/shutdown immediately. At least how about to print error message to
> reminder users.
> 
> 
> BTW, while I always do care about the loss of interrupt issue, the malicious
> device is able to hang a VM by dropping a single interrupt on purpose for
> virtio-scsi :)
> 
> 
> Thank you very much!


Can't say I understand what it's about. TDX does not protect against
hypervisor DoS attacks.

> Dongli Zhang
> 
> > 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.
> > 
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Paul E. McKenney <paulmck@kernel.org>
> > Signed-off-by: Jason Wang <jasowang@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,
> > 

Re: [PATCH V2 06/12] virtio_pci: harden MSI-X interrupts

[Jason Wang]

On Sat, Oct 16, 2021 at 1:27 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Oct 15, 2021 at 05:09:38AM -0700, Dongli Zhang wrote:
> > Hi Jason,
> >
> > On 10/11/21 11:52 PM, 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
> >
> > About "unexpected results", while you mentioned below in v1 ...
> >
> > "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."
> >
> > .. and you also mentioned to avoid the panic (due to untrusted device) in as
> > many scenarios as possible.
> >
> >
> > Here is my understanding.
> >
> > The reason we do not trust hypervisor is to protect (1) data/code privacy for
> > most of times and sometimes (2) program execution integrity.
> >
> > The bad thing is: the hypervisor is able to panic/destroy the VM in the worst case.
> >
> > It is reasonable to re-configure/recover if we assume there is BUG at
> > hypervisor/device side. That is, the hypervisor/device is buggy, but not malicious.
> >
> > However, how about to just detect and report the hypervisor/device is malicious
> > and shutdown/panic the VM immediately? If we have detected the malicious
> > hypervisor, we should avoid running VMs on the malicious hypervisor further. At
> > least how about to print error message to reminder users that the hypervisor is
> > malicious?

I understand your point, but several questions needs to be answered:

1) how can we easily differentiate "malicious" from "buggy"?
2) If we want to detect malicious hypervisor, virtio is not the only
place that we want to do this
3) Is there a way that "malicious" hypervisor can prevent guest from
shutting down itself?

> >
> >
> > About "unexpected results", it should not be hang/panic. I suggest ...
> >

It's just the phenomenon not the logic behind that. It could be e.g
OOB which may lead the unexpected kernel codes to be executed in
unexpected ways (e.g mark the page as shared or go with IOTLB etc).
Sometimes we can see panic finally but it's not always.

> > Assuming SEV/TDX is involved, the hypervisor should never be able to derive the
> > VM privacy (at least secure memory data) by providing malicious configuration,
> > e.g., num_queues=0.

Yes.

> > If we detect hypervisor is malicious, the VM is
> > panic/shutdown immediately.

This seems to enforce the policy into the kernel, we need to leave
this to userspace to decide.

> > At least how about to print error message to
> > reminder users.

This is fine.

> >
> >
> > BTW, while I always do care about the loss of interrupt issue, the malicious
> > device is able to hang a VM by dropping a single interrupt on purpose for
> > virtio-scsi :)
> >

Right.

> >
> > Thank you very much!
>
>
> Can't say I understand what it's about. TDX does not protect against
> hypervisor DoS attacks.

Yes, I think what Dongli wants to discuss is how to behave if we
detect a malicious hypervisor. He suggested a shutdown instead of
failing the probe.

Thanks

>
> > Dongli Zhang
> >
> > > 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.
> > >
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Paul E. McKenney <paulmck@kernel.org>
> > > Signed-off-by: Jason Wang <jasowang@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,
> > >
>

Re: [PATCH V2 06/12] virtio_pci: harden MSI-X interrupts

[Dongli Zhang]

On 10/18/21 6:33 PM, Jason Wang wrote:
> On Sat, Oct 16, 2021 at 1:27 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Fri, Oct 15, 2021 at 05:09:38AM -0700, Dongli Zhang wrote:
>>> Hi Jason,
>>>
>>> On 10/11/21 11:52 PM, 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
>>>
>>> About "unexpected results", while you mentioned below in v1 ...
>>>
>>> "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."
>>>
>>> .. and you also mentioned to avoid the panic (due to untrusted device) in as
>>> many scenarios as possible.
>>>
>>>
>>> Here is my understanding.
>>>
>>> The reason we do not trust hypervisor is to protect (1) data/code privacy for
>>> most of times and sometimes (2) program execution integrity.
>>>
>>> The bad thing is: the hypervisor is able to panic/destroy the VM in the worst case.
>>>
>>> It is reasonable to re-configure/recover if we assume there is BUG at
>>> hypervisor/device side. That is, the hypervisor/device is buggy, but not malicious.
>>>
>>> However, how about to just detect and report the hypervisor/device is malicious
>>> and shutdown/panic the VM immediately? If we have detected the malicious
>>> hypervisor, we should avoid running VMs on the malicious hypervisor further. At
>>> least how about to print error message to reminder users that the hypervisor is
>>> malicious?
> 
> I understand your point, but several questions needs to be answered:
> 
> 1) how can we easily differentiate "malicious" from "buggy"?
> 2) If we want to detect malicious hypervisor, virtio is not the only
> place that we want to do this
> 3) Is there a way that "malicious" hypervisor can prevent guest from
> shutting down itself?
> 
>>>
>>>
>>> About "unexpected results", it should not be hang/panic. I suggest ...
>>>
> 
> It's just the phenomenon not the logic behind that. It could be e.g
> OOB which may lead the unexpected kernel codes to be executed in
> unexpected ways (e.g mark the page as shared or go with IOTLB etc).
> Sometimes we can see panic finally but it's not always.

This is what I was trying to explain.

The objective is to protect "data privacy" (or code execution integrity in some
case), but not to prevent DoS attack. That is, the 'malicious' hypervisor should
not be able to derive VM data privacy.

Suppose the hypervisor did something buggy/malicious when SEV/TDX is used by VM,
the "unexpected results" should never reveal secure/private data to the hypervisor.

In my own opinion, the threat model is:

Attacker: 'malicious' hypervisor

Victim: VM with SEV/TDX/SGX

The attacker should not be able to steal secure/private data from VM, when the
hypervisor's action is unexpected. DoS is out of the scope.

My concern is: it is very hard to clearly explain in the patchset how the
hypervisor is able to steal VM's data, by setting queue=0 or injecting unwanted
interrupts to VM.

Thank you very much!

Dongli Zhang

> 
>>> Assuming SEV/TDX is involved, the hypervisor should never be able to derive the
>>> VM privacy (at least secure memory data) by providing malicious configuration,
>>> e.g., num_queues=0.
> 
> Yes.
> 
>>> If we detect hypervisor is malicious, the VM is
>>> panic/shutdown immediately.
> 
> This seems to enforce the policy into the kernel, we need to leave
> this to userspace to decide.
> 
>>> At least how about to print error message to
>>> reminder users.
> 
> This is fine.
> 
>>>
>>>
>>> BTW, while I always do care about the loss of interrupt issue, the malicious
>>> device is able to hang a VM by dropping a single interrupt on purpose for
>>> virtio-scsi :)
>>>
> 
> Right.
> 
>>>
>>> Thank you very much!
>>
>>
>> Can't say I understand what it's about. TDX does not protect against
>> hypervisor DoS attacks.
> 
> Yes, I think what Dongli wants to discuss is how to behave if we
> detect a malicious hypervisor. He suggested a shutdown instead of
> failing the probe.
> 
> Thanks
> 
>>
>>> Dongli Zhang
>>>
>>>> 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.
>>>>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: Paul E. McKenney <paulmck@kernel.org>
>>>> Signed-off-by: Jason Wang <jasowang@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,
>>>>
>>
> 

Re: [PATCH V2 06/12] virtio_pci: harden MSI-X interrupts

[Jason Wang]

On Wed, Oct 20, 2021 at 1:02 AM Dongli Zhang <dongli.zhang@oracle.com> wrote:
>
>
>
> On 10/18/21 6:33 PM, Jason Wang wrote:
> > On Sat, Oct 16, 2021 at 1:27 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >> On Fri, Oct 15, 2021 at 05:09:38AM -0700, Dongli Zhang wrote:
> >>> Hi Jason,
> >>>
> >>> On 10/11/21 11:52 PM, 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
> >>>
> >>> About "unexpected results", while you mentioned below in v1 ...
> >>>
> >>> "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."
> >>>
> >>> .. and you also mentioned to avoid the panic (due to untrusted device) in as
> >>> many scenarios as possible.
> >>>
> >>>
> >>> Here is my understanding.
> >>>
> >>> The reason we do not trust hypervisor is to protect (1) data/code privacy for
> >>> most of times and sometimes (2) program execution integrity.
> >>>
> >>> The bad thing is: the hypervisor is able to panic/destroy the VM in the worst case.
> >>>
> >>> It is reasonable to re-configure/recover if we assume there is BUG at
> >>> hypervisor/device side. That is, the hypervisor/device is buggy, but not malicious.
> >>>
> >>> However, how about to just detect and report the hypervisor/device is malicious
> >>> and shutdown/panic the VM immediately? If we have detected the malicious
> >>> hypervisor, we should avoid running VMs on the malicious hypervisor further. At
> >>> least how about to print error message to reminder users that the hypervisor is
> >>> malicious?
> >
> > I understand your point, but several questions needs to be answered:
> >
> > 1) how can we easily differentiate "malicious" from "buggy"?
> > 2) If we want to detect malicious hypervisor, virtio is not the only
> > place that we want to do this
> > 3) Is there a way that "malicious" hypervisor can prevent guest from
> > shutting down itself?
> >
> >>>
> >>>
> >>> About "unexpected results", it should not be hang/panic. I suggest ...
> >>>
> >
> > It's just the phenomenon not the logic behind that. It could be e.g
> > OOB which may lead the unexpected kernel codes to be executed in
> > unexpected ways (e.g mark the page as shared or go with IOTLB etc).
> > Sometimes we can see panic finally but it's not always.
>
> This is what I was trying to explain.
>
> The objective is to protect "data privacy" (or code execution integrity in some
> case), but not to prevent DoS attack. That is, the 'malicious' hypervisor should
> not be able to derive VM data privacy.
>
> Suppose the hypervisor did something buggy/malicious when SEV/TDX is used by VM,
> the "unexpected results" should never reveal secure/private data to the hypervisor.
>
> In my own opinion, the threat model is:
>
> Attacker: 'malicious' hypervisor
>
> Victim: VM with SEV/TDX/SGX
>
> The attacker should not be able to steal secure/private data from VM, when the
> hypervisor's action is unexpected. DoS is out of the scope.
>
> My concern is: it is very hard to clearly explain in the patchset how the
> hypervisor is able to steal VM's data, by setting queue=0 or injecting unwanted
> interrupts to VM.

Yes, it's a hard question but instead of trying to answer that, we can
just fix the case of e.g unexpected interrupts.

Thanks

>
> Thank you very much!
>
> Dongli Zhang
>
> >
> >>> Assuming SEV/TDX is involved, the hypervisor should never be able to derive the
> >>> VM privacy (at least secure memory data) by providing malicious configuration,
> >>> e.g., num_queues=0.
> >
> > Yes.
> >
> >>> If we detect hypervisor is malicious, the VM is
> >>> panic/shutdown immediately.
> >
> > This seems to enforce the policy into the kernel, we need to leave
> > this to userspace to decide.
> >
> >>> At least how about to print error message to
> >>> reminder users.
> >
> > This is fine.
> >
> >>>
> >>>
> >>> BTW, while I always do care about the loss of interrupt issue, the malicious
> >>> device is able to hang a VM by dropping a single interrupt on purpose for
> >>> virtio-scsi :)
> >>>
> >
> > Right.
> >
> >>>
> >>> Thank you very much!
> >>
> >>
> >> Can't say I understand what it's about. TDX does not protect against
> >> hypervisor DoS attacks.
> >
> > Yes, I think what Dongli wants to discuss is how to behave if we
> > detect a malicious hypervisor. He suggested a shutdown instead of
> > failing the probe.
> >
> > Thanks
> >
> >>
> >>> Dongli Zhang
> >>>
> >>>> 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.
> >>>>
> >>>> Cc: Thomas Gleixner <tglx@linutronix.de>
> >>>> Cc: Peter Zijlstra <peterz@infradead.org>
> >>>> Cc: Paul E. McKenney <paulmck@kernel.org>
> >>>> Signed-off-by: Jason Wang <jasowang@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,
> >>>>
> >>
> >
>

Re: [PATCH V2 06/12] virtio_pci: harden MSI-X interrupts

[Michael S. Tsirkin]

On Wed, Oct 20, 2021 at 09:33:49AM +0800, Jason Wang wrote:
> > In my own opinion, the threat model is:
> >
> > Attacker: 'malicious' hypervisor
> >
> > Victim: VM with SEV/TDX/SGX
> >
> > The attacker should not be able to steal secure/private data from VM, when the
> > hypervisor's action is unexpected. DoS is out of the scope.
> >
> > My concern is: it is very hard to clearly explain in the patchset how the
> > hypervisor is able to steal VM's data, by setting queue=0 or injecting unwanted
> > interrupts to VM.
> 
> Yes, it's a hard question but instead of trying to answer that, we can
> just fix the case of e.g unexpected interrupts.
> 
> Thanks

I think this it's still early days for TDX. So it's a bit early to talk
about threat models, start opening CVEs and distinguishing between
security and non-security bugs.

-- 
MST