[PATCH 00/11] virtio: Harden and test vring

[PATCH 00/11] virtio: Harden and test vring

[Andrew Scull]

Continuing the theme of making the virtio code resilient against
corruption of the buffers shared with the device, this series focusses
on the vring. This series is simpler and more self-contained than the
series for virtio-pci!

It follows the example of Linux by keeping a private copy of the
descriptors and metadata for state tracking and only ever writing to the
descriptors that are shared with the device.

I was able to test these hardening steps in the sandbox by simulating
device writes to the queues. I was also looking into testing the device
drivers against a simulated device but the lack of an API to access the
virtqueues meant this ended up being a hack. I've included that hack and
the at the end of the series as an RFC.

Andrew Scull (11):
  virtio_ring: Merge identical variables
  virtio_ring: Add helper to attach vring descriptor
  virtio_ring: Maintain a shadow copy of descriptors
  virtio_ring: Check used descriptors are chain heads
  dm: test: virtio: Test the virtio ring
  virtio: sandbox: Fix device features bitfield
  test: dm: virtio: Test notify before del_vqs
  virtio: sandbox: Bind RNG rather than block device
  test: dm: virtio: Test virtio device driver probing
  virtio: rng: Check length before copying
  RFC: test: dm: virtio: Test virtio-rng with faked device

 drivers/virtio/virtio_ring.c    |  90 ++++++++++++++--------
 drivers/virtio/virtio_rng.c     |   3 +
 drivers/virtio/virtio_sandbox.c |   4 +-
 include/virtio_ring.h           |  12 +++
 test/dm/virtio.c                | 129 ++++++++++++++++++++++++++++++--
 5 files changed, 199 insertions(+), 39 deletions(-)

-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 01/11] virtio_ring: Merge identical variables

[Andrew Scull]

The variables `total_sg` and `descs_used` have the same value. Replace
the few uses of `total_sg` with `descs_used` to simplify the situation.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_ring.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 7f1cbc5932..a6922ce1b8 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -20,17 +20,16 @@ int virtqueue_add(struct virtqueue *vq, struct virtio_sg *sgs[],
 		  unsigned int out_sgs, unsigned int in_sgs)
 {
 	struct vring_desc *desc;
-	unsigned int total_sg = out_sgs + in_sgs;
-	unsigned int i, n, avail, descs_used, uninitialized_var(prev);
+	unsigned int descs_used = out_sgs + in_sgs;
+	unsigned int i, n, avail, uninitialized_var(prev);
 	int head;
 
-	WARN_ON(total_sg == 0);
+	WARN_ON(descs_used == 0);
 
 	head = vq->free_head;
 
 	desc = vq->vring.desc;
 	i = head;
-	descs_used = total_sg;
 
 	if (vq->num_free < descs_used) {
 		debug("Can't add buf len %i - avail = %i\n",
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 02/11] virtio_ring: Add helper to attach vring descriptor

[Andrew Scull]

Move the logic for attaching a descriptor to its own function.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_ring.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a6922ce1b8..8e0cb3d666 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -16,6 +16,18 @@
 #include <linux/bug.h>
 #include <linux/compat.h>
 
+static unsigned int virtqueue_attach_desc(struct virtqueue *vq, unsigned int i,
+					  struct virtio_sg *sg, u16 flags)
+{
+	struct vring_desc *desc = &vq->vring.desc[i];
+
+	desc->addr = cpu_to_virtio64(vq->vdev, (u64)(uintptr_t)sg->addr);
+	desc->len = cpu_to_virtio32(vq->vdev, sg->length);
+	desc->flags = cpu_to_virtio16(vq->vdev, flags);
+
+	return virtio16_to_cpu(vq->vdev, desc->next);
+}
+
 int virtqueue_add(struct virtqueue *vq, struct virtio_sg *sgs[],
 		  unsigned int out_sgs, unsigned int in_sgs)
 {
@@ -45,26 +57,14 @@ int virtqueue_add(struct virtqueue *vq, struct virtio_sg *sgs[],
 	}
 
 	for (n = 0; n < out_sgs; n++) {
-		struct virtio_sg *sg = sgs[n];
-
-		desc[i].flags = cpu_to_virtio16(vq->vdev, VRING_DESC_F_NEXT);
-		desc[i].addr = cpu_to_virtio64(vq->vdev, (u64)(size_t)sg->addr);
-		desc[i].len = cpu_to_virtio32(vq->vdev, sg->length);
-
 		prev = i;
-		i = virtio16_to_cpu(vq->vdev, desc[i].next);
+		i = virtqueue_attach_desc(vq, i, sgs[n], VRING_DESC_F_NEXT);
 	}
 	for (; n < (out_sgs + in_sgs); n++) {
-		struct virtio_sg *sg = sgs[n];
-
-		desc[i].flags = cpu_to_virtio16(vq->vdev, VRING_DESC_F_NEXT |
-						VRING_DESC_F_WRITE);
-		desc[i].addr = cpu_to_virtio64(vq->vdev,
-					       (u64)(uintptr_t)sg->addr);
-		desc[i].len = cpu_to_virtio32(vq->vdev, sg->length);
+		u16 flags = VRING_DESC_F_NEXT | VRING_DESC_F_WRITE;
 
 		prev = i;
-		i = virtio16_to_cpu(vq->vdev, desc[i].next);
+		i = virtqueue_attach_desc(vq, i, sgs[n], flags);
 	}
 	/* Last one doesn't continue */
 	desc[prev].flags &= cpu_to_virtio16(vq->vdev, ~VRING_DESC_F_NEXT);
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 03/11] virtio_ring: Maintain a shadow copy of descriptors

[Andrew Scull]

The shared descriptors should only be written by the guest driver,
however, the device is still able to overwrite and corrupt them.
Maintain a private shadow copy of the descriptors for the driver to
use for state tracking, removing the need to read from the shared
descriptors.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_ring.c | 49 ++++++++++++++++++++++++------------
 include/virtio_ring.h        | 10 ++++++++
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 8e0cb3d666..69fd8c6aa0 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -19,13 +19,21 @@
 static unsigned int virtqueue_attach_desc(struct virtqueue *vq, unsigned int i,
 					  struct virtio_sg *sg, u16 flags)
 {
+	struct vring_desc_shadow *desc_shadow = &vq->vring_desc_shadow[i];
 	struct vring_desc *desc = &vq->vring.desc[i];
 
-	desc->addr = cpu_to_virtio64(vq->vdev, (u64)(uintptr_t)sg->addr);
-	desc->len = cpu_to_virtio32(vq->vdev, sg->length);
-	desc->flags = cpu_to_virtio16(vq->vdev, flags);
+	/* Update the shadow descriptor. */
+	desc_shadow->addr = (u64)(uintptr_t)sg->addr;
+	desc_shadow->len = sg->length;
+	desc_shadow->flags = flags;
 
-	return virtio16_to_cpu(vq->vdev, desc->next);
+	/* Update the shared descriptor to match the shadow. */
+	desc->addr = cpu_to_virtio64(vq->vdev, desc_shadow->addr);
+	desc->len = cpu_to_virtio32(vq->vdev, desc_shadow->len);
+	desc->flags = cpu_to_virtio16(vq->vdev, desc_shadow->flags);
+	desc->next = cpu_to_virtio16(vq->vdev, desc_shadow->next);
+
+	return desc_shadow->next;
 }
 
 int virtqueue_add(struct virtqueue *vq, struct virtio_sg *sgs[],
@@ -67,7 +75,8 @@ int virtqueue_add(struct virtqueue *vq, struct virtio_sg *sgs[],
 		i = virtqueue_attach_desc(vq, i, sgs[n], flags);
 	}
 	/* Last one doesn't continue */
-	desc[prev].flags &= cpu_to_virtio16(vq->vdev, ~VRING_DESC_F_NEXT);
+	vq->vring_desc_shadow[prev].flags &= ~VRING_DESC_F_NEXT;
+	desc[prev].flags = cpu_to_virtio16(vq->vdev, vq->vring_desc_shadow[prev].flags);
 
 	/* We're using some buffers from the free list. */
 	vq->num_free -= descs_used;
@@ -136,17 +145,16 @@ void virtqueue_kick(struct virtqueue *vq)
 static void detach_buf(struct virtqueue *vq, unsigned int head)
 {
 	unsigned int i;
-	__virtio16 nextflag = cpu_to_virtio16(vq->vdev, VRING_DESC_F_NEXT);
 
 	/* Put back on free list: unmap first-level descriptors and find end */
 	i = head;
 
-	while (vq->vring.desc[i].flags & nextflag) {
-		i = virtio16_to_cpu(vq->vdev, vq->vring.desc[i].next);
+	while (vq->vring_desc_shadow[i].flags & VRING_DESC_F_NEXT) {
+		i = vq->vring_desc_shadow[i].next;
 		vq->num_free++;
 	}
 
-	vq->vring.desc[i].next = cpu_to_virtio16(vq->vdev, vq->free_head);
+	vq->vring_desc_shadow[i].next = vq->free_head;
 	vq->free_head = head;
 
 	/* Plus final descriptor */
@@ -199,8 +207,7 @@ void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len)
 		virtio_store_mb(&vring_used_event(&vq->vring),
 				cpu_to_virtio16(vq->vdev, vq->last_used_idx));
 
-	return (void *)(uintptr_t)virtio64_to_cpu(vq->vdev,
-						  vq->vring.desc[i].addr);
+	return (void *)(uintptr_t)vq->vring_desc_shadow[i].addr;
 }
 
 static struct virtqueue *__vring_new_virtqueue(unsigned int index,
@@ -209,6 +216,7 @@ static struct virtqueue *__vring_new_virtqueue(unsigned int index,
 {
 	unsigned int i;
 	struct virtqueue *vq;
+	struct vring_desc_shadow *vring_desc_shadow;
 	struct virtio_dev_priv *uc_priv = dev_get_uclass_priv(udev);
 	struct udevice *vdev = uc_priv->vdev;
 
@@ -216,10 +224,17 @@ static struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	if (!vq)
 		return NULL;
 
+	vring_desc_shadow = calloc(vring.num, sizeof(struct vring_desc_shadow));
+	if (!vring_desc_shadow) {
+		free(vq);
+		return NULL;
+	}
+
 	vq->vdev = vdev;
 	vq->index = index;
 	vq->num_free = vring.num;
 	vq->vring = vring;
+	vq->vring_desc_shadow = vring_desc_shadow;
 	vq->last_used_idx = 0;
 	vq->avail_flags_shadow = 0;
 	vq->avail_idx_shadow = 0;
@@ -237,7 +252,7 @@ static struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	/* Put everything in free lists */
 	vq->free_head = 0;
 	for (i = 0; i < vring.num - 1; i++)
-		vq->vring.desc[i].next = cpu_to_virtio16(vdev, i + 1);
+		vq->vring_desc_shadow[i].next = i + 1;
 
 	return vq;
 }
@@ -290,6 +305,7 @@ struct virtqueue *vring_create_virtqueue(unsigned int index, unsigned int num,
 void vring_del_virtqueue(struct virtqueue *vq)
 {
 	free(vq->vring.desc);
+	free(vq->vring_desc_shadow);
 	list_del(&vq->list);
 	free(vq);
 }
@@ -335,11 +351,12 @@ void virtqueue_dump(struct virtqueue *vq)
 	printf("\tlast_used_idx %u, avail_flags_shadow %u, avail_idx_shadow %u\n",
 	       vq->last_used_idx, vq->avail_flags_shadow, vq->avail_idx_shadow);
 
-	printf("Descriptor dump:\n");
+	printf("Shadow descriptor dump:\n");
 	for (i = 0; i < vq->vring.num; i++) {
-		printf("\tdesc[%u] = { 0x%llx, len %u, flags %u, next %u }\n",
-		       i, vq->vring.desc[i].addr, vq->vring.desc[i].len,
-		       vq->vring.desc[i].flags, vq->vring.desc[i].next);
+		struct vring_desc_shadow *desc = &vq->vring_desc_shadow[i];
+
+		printf("\tdesc_shadow[%u] = { 0x%llx, len %u, flags %u, next %u }\n",
+		       i, desc->addr, desc->len, desc->flags, desc->next);
 	}
 
 	printf("Avail ring dump:\n");
diff --git a/include/virtio_ring.h b/include/virtio_ring.h
index 6fc0593b14..52cbe77c0a 100644
--- a/include/virtio_ring.h
+++ b/include/virtio_ring.h
@@ -55,6 +55,14 @@ struct vring_desc {
 	__virtio16 next;
 };
 
+/* Shadow of struct vring_desc in guest byte order. */
+struct vring_desc_shadow {
+	u64 addr;
+	u32 len;
+	u16 flags;
+	u16 next;
+};
+
 struct vring_avail {
 	__virtio16 flags;
 	__virtio16 idx;
@@ -89,6 +97,7 @@ struct vring {
  * @index: the zero-based ordinal number for this queue
  * @num_free: number of elements we expect to be able to fit
  * @vring: actual memory layout for this queue
+ * @vring_desc_shadow: guest-only copy of descriptors
  * @event: host publishes avail event idx
  * @free_head: head of free buffer list
  * @num_added: number we've added since last sync
@@ -102,6 +111,7 @@ struct virtqueue {
 	unsigned int index;
 	unsigned int num_free;
 	struct vring vring;
+	struct vring_desc_shadow *vring_desc_shadow;
 	bool event;
 	unsigned int free_head;
 	unsigned int num_added;
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 04/11] virtio_ring: Check used descriptors are chain heads

[Andrew Scull]

When the device returns used buffers, it should refer to the descriptor
that is the head of the descriptor chain for that buffer. Confirm this
to be the case by tracking the head of descriptor chains that have been
made available to the device.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_ring.c | 12 ++++++++++++
 include/virtio_ring.h        |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 69fd8c6aa0..383d574cb0 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -84,6 +84,9 @@ int virtqueue_add(struct virtqueue *vq, struct virtio_sg *sgs[],
 	/* Update free pointer */
 	vq->free_head = i;
 
+	/* Mark the descriptor as the head of a chain. */
+	vq->vring_desc_shadow[head].chain_head = true;
+
 	/*
 	 * Put entry in available array (but don't update avail->idx
 	 * until they do sync).
@@ -146,6 +149,9 @@ static void detach_buf(struct virtqueue *vq, unsigned int head)
 {
 	unsigned int i;
 
+	/* Unmark the descriptor as the head of a chain. */
+	vq->vring_desc_shadow[head].chain_head = false;
+
 	/* Put back on free list: unmap first-level descriptors and find end */
 	i = head;
 
@@ -196,6 +202,12 @@ void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len)
 		return NULL;
 	}
 
+	if (unlikely(!vq->vring_desc_shadow[i].chain_head)) {
+		printf("(%s.%d): id %u is not a head\n",
+		       vq->vdev->name, vq->index, i);
+		return NULL;
+	}
+
 	detach_buf(vq, i);
 	vq->last_used_idx++;
 	/*
diff --git a/include/virtio_ring.h b/include/virtio_ring.h
index 52cbe77c0a..c77c212cff 100644
--- a/include/virtio_ring.h
+++ b/include/virtio_ring.h
@@ -61,6 +61,8 @@ struct vring_desc_shadow {
 	u32 len;
 	u16 flags;
 	u16 next;
+	/* Metadata about the descriptor. */
+	bool chain_head;
 };
 
 struct vring_avail {
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 05/11] dm: test: virtio: Test the virtio ring

[Andrew Scull]

The virtio ring is the basis of virtio communication. Test its basic
functionality and its resilience against corruption from the device.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 test/dm/virtio.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/test/dm/virtio.c b/test/dm/virtio.c
index 9a7e658cce..4cf0f56260 100644
--- a/test/dm/virtio.c
+++ b/test/dm/virtio.c
@@ -130,3 +130,75 @@ static int dm_test_virtio_remove(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_virtio_remove, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+/* Test all of the virtio ring */
+static int dm_test_virtio_ring(struct unit_test_state *uts)
+{
+	struct udevice *bus, *dev;
+	struct virtio_dev_priv *uc_priv;
+	struct virtqueue *vq;
+	struct virtio_sg sg[2];
+	struct virtio_sg *sgs[2];
+	unsigned int len;
+	u8 buffer[2][32];
+
+	/* check probe success */
+	ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
+	ut_assertnonnull(bus);
+
+	/* check the child virtio-rng device is bound */
+	ut_assertok(device_find_first_child(bus, &dev));
+	ut_assertnonnull(dev);
+
+	/*
+	 * fake the virtio device probe by filling in uc_priv->vdev
+	 * which is used by virtio_find_vqs/virtio_del_vqs.
+	 */
+	uc_priv = dev_get_uclass_priv(bus);
+	ut_assertnonnull(uc_priv);
+	uc_priv->vdev = dev;
+
+	/* prepare the scatter-gather buffer */
+	sg[0].addr = buffer[0];
+	sg[0].length = sizeof(buffer[0]);
+	sg[1].addr = buffer[1];
+	sg[1].length = sizeof(buffer[1]);
+	sgs[0] = &sg[0];
+	sgs[1] = &sg[1];
+
+	/* read a buffer and report written size from device */
+	ut_assertok(virtio_find_vqs(dev, 1, &vq));
+	ut_assertok(virtqueue_add(vq, sgs, 0, 1));
+	vq->vring.used->idx = 1;
+	vq->vring.used->ring[0].id = 0;
+	vq->vring.used->ring[0].len = 0x53355885;
+	ut_asserteq_ptr(buffer, virtqueue_get_buf(vq, &len));
+	ut_asserteq(0x53355885, len);
+	ut_assertok(virtio_del_vqs(dev));
+
+	/* rejects used descriptors that aren't a chain head */
+	ut_assertok(virtio_find_vqs(dev, 1, &vq));
+	ut_assertok(virtqueue_add(vq, sgs, 0, 2));
+	vq->vring.used->idx = 1;
+	vq->vring.used->ring[0].id = 1;
+	vq->vring.used->ring[0].len = 0x53355885;
+	ut_assertnull(virtqueue_get_buf(vq, &len));
+	ut_assertok(virtio_del_vqs(dev));
+
+	/* device changes to descriptor are ignored */
+	ut_assertok(virtio_find_vqs(dev, 1, &vq));
+	ut_assertok(virtqueue_add(vq, sgs, 0, 1));
+	vq->vring.desc[0].addr = cpu_to_virtio64(dev, 0xbadbad11);
+	vq->vring.desc[0].len = cpu_to_virtio32(dev, 0x11badbad);
+	vq->vring.desc[0].flags = cpu_to_virtio16(dev, VRING_DESC_F_NEXT);
+	vq->vring.desc[0].next = cpu_to_virtio16(dev, U16_MAX);
+	vq->vring.used->idx = 1;
+	vq->vring.used->ring[0].id = 0;
+	vq->vring.used->ring[0].len = 6;
+	ut_asserteq_ptr(buffer, virtqueue_get_buf(vq, &len));
+	ut_asserteq(6, len);
+	ut_assertok(virtio_del_vqs(dev));
+
+	return 0;
+}
+DM_TEST(dm_test_virtio_ring, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 06/11] virtio: sandbox: Fix device features bitfield

[Andrew Scull]

The virtio sandbox transport was setting the device features value to
the bit index rather than shifting a bit to the right index. Fix this
using the bit manipulation macros.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_sandbox.c | 2 +-
 test/dm/virtio.c                | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_sandbox.c b/drivers/virtio/virtio_sandbox.c
index aafb7beb94..a73b123454 100644
--- a/drivers/virtio/virtio_sandbox.c
+++ b/drivers/virtio/virtio_sandbox.c
@@ -160,7 +160,7 @@ static int virtio_sandbox_probe(struct udevice *udev)
 	struct virtio_dev_priv *uc_priv = dev_get_uclass_priv(udev);
 
 	/* fake some information for testing */
-	priv->device_features = VIRTIO_F_VERSION_1;
+	priv->device_features = BIT_ULL(VIRTIO_F_VERSION_1);
 	uc_priv->device = VIRTIO_ID_BLOCK;
 	uc_priv->vendor = ('u' << 24) | ('b' << 16) | ('o' << 8) | 't';
 
diff --git a/test/dm/virtio.c b/test/dm/virtio.c
index 4cf0f56260..63fcc22172 100644
--- a/test/dm/virtio.c
+++ b/test/dm/virtio.c
@@ -77,7 +77,7 @@ static int dm_test_virtio_all_ops(struct unit_test_state *uts)
 	ut_assertok(virtio_get_status(dev, &status));
 	ut_asserteq(0, status);
 	ut_assertok(virtio_get_features(dev, &features));
-	ut_asserteq(VIRTIO_F_VERSION_1, features);
+	ut_asserteq_64(BIT_ULL(VIRTIO_F_VERSION_1), features);
 	ut_assertok(virtio_set_features(dev));
 	ut_assertok(virtio_find_vqs(dev, nvqs, vqs));
 	ut_assertok(virtio_del_vqs(dev));
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 07/11] test: dm: virtio: Test notify before del_vqs

[Andrew Scull]

The virtqueue is passed to virtio_notify() so move the virtqueue
deletion to the end of the test when it's no longer needed. This wasn't
causing any problems because the sandbox virtio transport driver doesn't
do anything for notifications, but it could cause problems if things
change and it was a bad example.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 test/dm/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/dm/virtio.c b/test/dm/virtio.c
index 63fcc22172..d054ccfaa4 100644
--- a/test/dm/virtio.c
+++ b/test/dm/virtio.c
@@ -80,8 +80,8 @@ static int dm_test_virtio_all_ops(struct unit_test_state *uts)
 	ut_asserteq_64(BIT_ULL(VIRTIO_F_VERSION_1), features);
 	ut_assertok(virtio_set_features(dev));
 	ut_assertok(virtio_find_vqs(dev, nvqs, vqs));
-	ut_assertok(virtio_del_vqs(dev));
 	ut_assertok(virtio_notify(dev, vqs[0]));
+	ut_assertok(virtio_del_vqs(dev));
 
 	return 0;
 }
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 08/11] virtio: sandbox: Bind RNG rather than block device

[Andrew Scull]

The virtio-rng driver is extremely simple, making it suitable for
testing more of the virtio uclass logic. Have the sandbox driver bind
the virtio-rng driver rather than the virtio-blk driver so it can be
used in tests.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_sandbox.c | 2 +-
 test/dm/virtio.c                | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_sandbox.c b/drivers/virtio/virtio_sandbox.c
index a73b123454..5484ae3a1a 100644
--- a/drivers/virtio/virtio_sandbox.c
+++ b/drivers/virtio/virtio_sandbox.c
@@ -161,7 +161,7 @@ static int virtio_sandbox_probe(struct udevice *udev)
 
 	/* fake some information for testing */
 	priv->device_features = BIT_ULL(VIRTIO_F_VERSION_1);
-	uc_priv->device = VIRTIO_ID_BLOCK;
+	uc_priv->device = VIRTIO_ID_RNG;
 	uc_priv->vendor = ('u' << 24) | ('b' << 16) | ('o' << 8) | 't';
 
 	return 0;
diff --git a/test/dm/virtio.c b/test/dm/virtio.c
index d054ccfaa4..769945a0d8 100644
--- a/test/dm/virtio.c
+++ b/test/dm/virtio.c
@@ -25,10 +25,10 @@ static int dm_test_virtio_base(struct unit_test_state *uts)
 	ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
 	ut_assertnonnull(bus);
 
-	/* check the child virtio-blk device is bound */
+	/* check the child virtio-rng device is bound */
 	ut_assertok(device_find_first_child(bus, &dev));
 	ut_assertnonnull(dev);
-	ut_assertok(strcmp(dev->name, "virtio-blk#0"));
+	ut_asserteq_str("virtio-rng#0", dev->name);
 
 	/* check driver status */
 	ut_assertok(virtio_get_status(dev, &status));
@@ -54,7 +54,7 @@ static int dm_test_virtio_all_ops(struct unit_test_state *uts)
 	ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
 	ut_assertnonnull(bus);
 
-	/* check the child virtio-blk device is bound */
+	/* check the child virtio-rng device is bound */
 	ut_assertok(device_find_first_child(bus, &dev));
 	ut_assertnonnull(dev);
 
@@ -114,7 +114,7 @@ static int dm_test_virtio_remove(struct unit_test_state *uts)
 	ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
 	ut_assertnonnull(bus);
 
-	/* check the child virtio-blk device is bound */
+	/* check the child virtio-rng device is bound */
 	ut_assertok(device_find_first_child(bus, &dev));
 	ut_assertnonnull(dev);
 
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 09/11] test: dm: virtio: Test virtio device driver probing

[Andrew Scull]

Once the virtio-rng driver has been bound, probe it to trigger the pre
and post child probe hooks of the virtio uclass driver. Check the status
of the virtio device to confirm it reached the expected state.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 test/dm/virtio.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/dm/virtio.c b/test/dm/virtio.c
index 769945a0d8..7139c31ab5 100644
--- a/test/dm/virtio.c
+++ b/test/dm/virtio.c
@@ -34,6 +34,15 @@ static int dm_test_virtio_base(struct unit_test_state *uts)
 	ut_assertok(virtio_get_status(dev, &status));
 	ut_asserteq(VIRTIO_CONFIG_S_ACKNOWLEDGE, status);
 
+	/* probe the virtio-rng driver */
+	ut_assertok(device_probe(dev));
+
+	/* check the device was reset and the driver picked up the device */
+	ut_assertok(virtio_get_status(dev, &status));
+	ut_asserteq(VIRTIO_CONFIG_S_DRIVER |
+		    VIRTIO_CONFIG_S_DRIVER_OK |
+		    VIRTIO_CONFIG_S_FEATURES_OK, status);
+
 	return 0;
 }
 DM_TEST(dm_test_virtio_base, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 10/11] virtio: rng: Check length before copying

[Andrew Scull]

Check the length of data written by the device is consistent with the
size of the buffers to avoid out-of-bounds memory accesses in case
values aren't consistent.

Signed-off-by: Andrew Scull <ascull@google.com>
Cc: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 drivers/virtio/virtio_rng.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
index 9314c0a03e..b85545c2ee 100644
--- a/drivers/virtio/virtio_rng.c
+++ b/drivers/virtio/virtio_rng.c
@@ -41,6 +41,9 @@ static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
 		while (!virtqueue_get_buf(priv->rng_vq, &rsize))
 			;
 
+		if (rsize > sg.length)
+			return -EIO;
+
 		memcpy(ptr, buf, rsize);
 		len -= rsize;
 		ptr += rsize;
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 11/11] RFC: test: dm: virtio: Test virtio-rng with faked device

[Andrew Scull]

When looking into possibilities for testing virtio drivers I was trying
to simulate the device's responses in the virtqueue. It required a hack
to get access to the virtqueue by accessing the driver's private data
and only allows pre-programmed buffer returns but no dynamic responses,
data or descriptor modifications.

This is an example of a regression test for the virtio-rng fix in the
previous patch.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 test/dm/virtio.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/test/dm/virtio.c b/test/dm/virtio.c
index 7139c31ab5..85791e3f58 100644
--- a/test/dm/virtio.c
+++ b/test/dm/virtio.c
@@ -12,6 +12,7 @@
 #include <dm/root.h>
 #include <dm/test.h>
 #include <dm/uclass-internal.h>
+#include <rng.h>
 #include <test/test.h>
 #include <test/ut.h>
 
@@ -211,3 +212,38 @@ static int dm_test_virtio_ring(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_virtio_ring, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+struct virtio_rng_priv {
+	struct virtqueue *rng_vq;
+};
+
+/* Test the virtio-rng driver validates the used size */
+static int dm_test_virtio_rng(struct unit_test_state *uts)
+{
+	struct udevice *bus, *dev;
+	struct virtio_rng_priv *priv;
+	u8 buffer[16];
+
+	/* check probe success */
+	ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
+	ut_assertnonnull(bus);
+
+	/* check the child virtio-rng device is bound */
+	ut_assertok(device_find_first_child(bus, &dev));
+	ut_assertnonnull(dev);
+
+	/* probe the virtio-rng driver */
+	ut_assertok(device_probe(dev));
+
+	/* simulate the device returning the buffer with too much data */
+	priv = dev_get_priv(dev);
+	priv->rng_vq->vring.used->idx = 1;
+	priv->rng_vq->vring.used->ring[0].id = 0;
+	priv->rng_vq->vring.used->ring[0].len = U32_MAX;
+
+	/* check the driver gracefully handles the error */
+	ut_asserteq(-EIO, dm_rng_read(dev, buffer, sizeof(buffer)));
+
+	return 0;
+}
+DM_TEST(dm_test_virtio_rng, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.35.1.1094.g7c7d902a7c-goog

Re: [PATCH 10/11] virtio: rng: Check length before copying

[Pierre-Clément Tosi]

Hi,

On Thu, Mar 31, 2022 at 10:09:48AM +0000, Andrew Scull wrote:
> Check the length of data written by the device is consistent with the
> size of the buffers to avoid out-of-bounds memory accesses in case
> values aren't consistent.
> 
> Signed-off-by: Andrew Scull <ascull@google.com>
> Cc: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  drivers/virtio/virtio_rng.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
> index 9314c0a03e..b85545c2ee 100644
> --- a/drivers/virtio/virtio_rng.c
> +++ b/drivers/virtio/virtio_rng.c
> @@ -41,6 +41,9 @@ static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
>  		while (!virtqueue_get_buf(priv->rng_vq, &rsize))
>  			;
>  
> +		if (rsize > sg.length)
> +			return -EIO;
> +

Although this patch addresses a legitimate concern, could we instead aim for
strengthening the lower-level virtio building blocks (e.g. virtqueue_get_buf())
so that higher-level virtio device drivers such as virtio-{rng,console,net,...}
don't have to be littered with checks of this nature? Could this be achieved by
using the shadow copy introduced in [PATCH 03/11]?

>  		memcpy(ptr, buf, rsize);
>  		len -= rsize;
>  		ptr += rsize;
> -- 
> 2.35.1.1094.g7c7d902a7c-goog
> 

Thanks,

-- 
Pierre

Re: [PATCH 01/11] virtio_ring: Merge identical variables

[Heinrich Schuchardt]

On 3/31/22 12:09, Andrew Scull wrote:
> The variables `total_sg` and `descs_used` have the same value. Replace
> the few uses of `total_sg` with `descs_used` to simplify the situation.
>
> Signed-off-by: Andrew Scull <ascull@google.com>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   drivers/virtio/virtio_ring.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 7f1cbc5932..a6922ce1b8 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -20,17 +20,16 @@ int virtqueue_add(struct virtqueue *vq, struct virtio_sg *sgs[],
>   		  unsigned int out_sgs, unsigned int in_sgs)
>   {
>   	struct vring_desc *desc;
> -	unsigned int total_sg = out_sgs + in_sgs;
> -	unsigned int i, n, avail, descs_used, uninitialized_var(prev);
> +	unsigned int descs_used = out_sgs + in_sgs;
> +	unsigned int i, n, avail, uninitialized_var(prev);
>   	int head;
>
> -	WARN_ON(total_sg == 0);
> +	WARN_ON(descs_used == 0);
>
>   	head = vq->free_head;
>
>   	desc = vq->vring.desc;
>   	i = head;
> -	descs_used = total_sg;
>
>   	if (vq->num_free < descs_used) {
>   		debug("Can't add buf len %i - avail = %i\n",

Re: [PATCH 08/11] virtio: sandbox: Bind RNG rather than block device

[Heinrich Schuchardt]

On 3/31/22 12:09, Andrew Scull wrote:
> The virtio-rng driver is extremely simple, making it suitable for
> testing more of the virtio uclass logic. Have the sandbox driver bind
> the virtio-rng driver rather than the virtio-blk driver so it can be
> used in tests.

test/dm/rng.c assumes that drivers/rng/sandbox_rng.c is providing the
only RNG device.

Does test/dm/virtio.c guarantee that no virtio-rng device is bound after
the test is run?

Best regards

Heinrich

>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>   drivers/virtio/virtio_sandbox.c | 2 +-
>   test/dm/virtio.c                | 8 ++++----
>   2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/virtio/virtio_sandbox.c b/drivers/virtio/virtio_sandbox.c
> index a73b123454..5484ae3a1a 100644
> --- a/drivers/virtio/virtio_sandbox.c
> +++ b/drivers/virtio/virtio_sandbox.c
> @@ -161,7 +161,7 @@ static int virtio_sandbox_probe(struct udevice *udev)
>
>   	/* fake some information for testing */
>   	priv->device_features = BIT_ULL(VIRTIO_F_VERSION_1);
> -	uc_priv->device = VIRTIO_ID_BLOCK;
> +	uc_priv->device = VIRTIO_ID_RNG;
>   	uc_priv->vendor = ('u' << 24) | ('b' << 16) | ('o' << 8) | 't';
>
>   	return 0;
> diff --git a/test/dm/virtio.c b/test/dm/virtio.c
> index d054ccfaa4..769945a0d8 100644
> --- a/test/dm/virtio.c
> +++ b/test/dm/virtio.c
> @@ -25,10 +25,10 @@ static int dm_test_virtio_base(struct unit_test_state *uts)
>   	ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
>   	ut_assertnonnull(bus);
>
> -	/* check the child virtio-blk device is bound */
> +	/* check the child virtio-rng device is bound */
>   	ut_assertok(device_find_first_child(bus, &dev));
>   	ut_assertnonnull(dev);
> -	ut_assertok(strcmp(dev->name, "virtio-blk#0"));
> +	ut_asserteq_str("virtio-rng#0", dev->name);
>
>   	/* check driver status */
>   	ut_assertok(virtio_get_status(dev, &status));
> @@ -54,7 +54,7 @@ static int dm_test_virtio_all_ops(struct unit_test_state *uts)
>   	ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
>   	ut_assertnonnull(bus);
>
> -	/* check the child virtio-blk device is bound */
> +	/* check the child virtio-rng device is bound */
>   	ut_assertok(device_find_first_child(bus, &dev));
>   	ut_assertnonnull(dev);
>
> @@ -114,7 +114,7 @@ static int dm_test_virtio_remove(struct unit_test_state *uts)
>   	ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
>   	ut_assertnonnull(bus);
>
> -	/* check the child virtio-blk device is bound */
> +	/* check the child virtio-rng device is bound */
>   	ut_assertok(device_find_first_child(bus, &dev));
>   	ut_assertnonnull(dev);
>

Re: [PATCH 10/11] virtio: rng: Check length before copying

[Andrew Scull]

On Wed, 6 Apr 2022 at 15:18, Pierre-Clément Tosi <ptosi@google.com> wrote:
>
> Hi,
>
> On Thu, Mar 31, 2022 at 10:09:48AM +0000, Andrew Scull wrote:
> > Check the length of data written by the device is consistent with the
> > size of the buffers to avoid out-of-bounds memory accesses in case
> > values aren't consistent.
> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > Cc: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  drivers/virtio/virtio_rng.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
> > index 9314c0a03e..b85545c2ee 100644
> > --- a/drivers/virtio/virtio_rng.c
> > +++ b/drivers/virtio/virtio_rng.c
> > @@ -41,6 +41,9 @@ static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
> >               while (!virtqueue_get_buf(priv->rng_vq, &rsize))
> >                       ;
> >
> > +             if (rsize > sg.length)
> > +                     return -EIO;
> > +
>
> Although this patch addresses a legitimate concern, could we instead aim for
> strengthening the lower-level virtio building blocks (e.g. virtqueue_get_buf())
> so that higher-level virtio device drivers such as virtio-{rng,console,net,...}
> don't have to be littered with checks of this nature? Could this be achieved by
> using the shadow copy introduced in [PATCH 03/11]?

There could certainly be _a_ bounds check in the vring driver, to
check that the total size written doesn't exceed the cumulative size
of the writable buffers in the descriptor chain. That would be
satisfactory for this rng driver, since there is only one buffer being
used, but if there is more than one buffer then the device driver will
still need to do checks as it accesses each of them. So it still feels
like the device driver's responsibility to do the checking, given the
current API.

> >               memcpy(ptr, buf, rsize);
> >               len -= rsize;
> >               ptr += rsize;
> > --
> > 2.35.1.1094.g7c7d902a7c-goog
> >
>
> Thanks,
>
> --
> Pierre

Re: [PATCH 08/11] virtio: sandbox: Bind RNG rather than block device

[Andrew Scull]

On Thu, 7 Apr 2022 at 08:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/31/22 12:09, Andrew Scull wrote:
> > The virtio-rng driver is extremely simple, making it suitable for
> > testing more of the virtio uclass logic. Have the sandbox driver bind
> > the virtio-rng driver rather than the virtio-blk driver so it can be
> > used in tests.
>
> test/dm/rng.c assumes that drivers/rng/sandbox_rng.c is providing the
> only RNG device.
>
> Does test/dm/virtio.c guarantee that no virtio-rng device is bound after
> the test is run?

My understanding was that dm_test_pre_run() in test/test-main.c reset
the driver model for each dm test, which would imply that nothing is
bound at the start of the test. Have I understood this correctly?

> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >   drivers/virtio/virtio_sandbox.c | 2 +-
> >   test/dm/virtio.c                | 8 ++++----
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_sandbox.c b/drivers/virtio/virtio_sandbox.c
> > index a73b123454..5484ae3a1a 100644
> > --- a/drivers/virtio/virtio_sandbox.c
> > +++ b/drivers/virtio/virtio_sandbox.c
> > @@ -161,7 +161,7 @@ static int virtio_sandbox_probe(struct udevice *udev)
> >
> >       /* fake some information for testing */
> >       priv->device_features = BIT_ULL(VIRTIO_F_VERSION_1);
> > -     uc_priv->device = VIRTIO_ID_BLOCK;
> > +     uc_priv->device = VIRTIO_ID_RNG;
> >       uc_priv->vendor = ('u' << 24) | ('b' << 16) | ('o' << 8) | 't';
> >
> >       return 0;
> > diff --git a/test/dm/virtio.c b/test/dm/virtio.c
> > index d054ccfaa4..769945a0d8 100644
> > --- a/test/dm/virtio.c
> > +++ b/test/dm/virtio.c
> > @@ -25,10 +25,10 @@ static int dm_test_virtio_base(struct unit_test_state *uts)
> >       ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
> >       ut_assertnonnull(bus);
> >
> > -     /* check the child virtio-blk device is bound */
> > +     /* check the child virtio-rng device is bound */
> >       ut_assertok(device_find_first_child(bus, &dev));
> >       ut_assertnonnull(dev);
> > -     ut_assertok(strcmp(dev->name, "virtio-blk#0"));
> > +     ut_asserteq_str("virtio-rng#0", dev->name);
> >
> >       /* check driver status */
> >       ut_assertok(virtio_get_status(dev, &status));
> > @@ -54,7 +54,7 @@ static int dm_test_virtio_all_ops(struct unit_test_state *uts)
> >       ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
> >       ut_assertnonnull(bus);
> >
> > -     /* check the child virtio-blk device is bound */
> > +     /* check the child virtio-rng device is bound */
> >       ut_assertok(device_find_first_child(bus, &dev));
> >       ut_assertnonnull(dev);
> >
> > @@ -114,7 +114,7 @@ static int dm_test_virtio_remove(struct unit_test_state *uts)
> >       ut_assertok(uclass_first_device(UCLASS_VIRTIO, &bus));
> >       ut_assertnonnull(bus);
> >
> > -     /* check the child virtio-blk device is bound */
> > +     /* check the child virtio-rng device is bound */
> >       ut_assertok(device_find_first_child(bus, &dev));
> >       ut_assertnonnull(dev);
> >
>

Re: [PATCH 02/11] virtio_ring: Add helper to attach vring descriptor

[Simon Glass]

On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
>
> Move the logic for attaching a descriptor to its own function.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_ring.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 03/11] virtio_ring: Maintain a shadow copy of descriptors

[Simon Glass]

On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
>
> The shared descriptors should only be written by the guest driver,
> however, the device is still able to overwrite and corrupt them.
> Maintain a private shadow copy of the descriptors for the driver to
> use for state tracking, removing the need to read from the shared
> descriptors.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_ring.c | 49 ++++++++++++++++++++++++------------
>  include/virtio_ring.h        | 10 ++++++++
>  2 files changed, 43 insertions(+), 16 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 04/11] virtio_ring: Check used descriptors are chain heads

[Simon Glass]

On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
>
> When the device returns used buffers, it should refer to the descriptor
> that is the head of the descriptor chain for that buffer. Confirm this
> to be the case by tracking the head of descriptor chains that have been
> made available to the device.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_ring.c | 12 ++++++++++++
>  include/virtio_ring.h        |  2 ++
>  2 files changed, 14 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 08/11] virtio: sandbox: Bind RNG rather than block device

[Simon Glass]

Hi Andrew,

On Thu, 7 Apr 2022 at 04:16, Andrew Scull <ascull@google.com> wrote:
>
> On Thu, 7 Apr 2022 at 08:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 3/31/22 12:09, Andrew Scull wrote:
> > > The virtio-rng driver is extremely simple, making it suitable for
> > > testing more of the virtio uclass logic. Have the sandbox driver bind
> > > the virtio-rng driver rather than the virtio-blk driver so it can be
> > > used in tests.
> >
> > test/dm/rng.c assumes that drivers/rng/sandbox_rng.c is providing the
> > only RNG device.
> >
> > Does test/dm/virtio.c guarantee that no virtio-rng device is bound after
> > the test is run?
>
> My understanding was that dm_test_pre_run() in test/test-main.c reset
> the driver model for each dm test, which would imply that nothing is
> bound at the start of the test. Have I understood this correctly?

Yes.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Re: [PATCH 11/11] RFC: test: dm: virtio: Test virtio-rng with faked device

[Simon Glass]

On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
>
> When looking into possibilities for testing virtio drivers I was trying
> to simulate the device's responses in the virtqueue. It required a hack
> to get access to the virtqueue by accessing the driver's private data
> and only allows pre-programmed buffer returns but no dynamic responses,
> data or descriptor modifications.
>
> This is an example of a regression test for the virtio-rng fix in the
> previous patch.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  test/dm/virtio.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Seems fine to me.

Re: [PATCH 01/11] virtio_ring: Merge identical variables

[Simon Glass]

On Thu, 7 Apr 2022 at 01:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/31/22 12:09, Andrew Scull wrote:
> > The variables `total_sg` and `descs_used` have the same value. Replace
> > the few uses of `total_sg` with `descs_used` to simplify the situation.
> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 10/11] virtio: rng: Check length before copying

[Simon Glass]

On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
>
> Check the length of data written by the device is consistent with the
> size of the buffers to avoid out-of-bounds memory accesses in case
> values aren't consistent.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> Cc: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  drivers/virtio/virtio_rng.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 05/11] dm: test: virtio: Test the virtio ring

[Simon Glass]

On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
>
> The virtio ring is the basis of virtio communication. Test its basic
> functionality and its resilience against corruption from the device.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  test/dm/virtio.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 06/11] virtio: sandbox: Fix device features bitfield

[Simon Glass]

On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
>
> The virtio sandbox transport was setting the device features value to
> the bit index rather than shifting a bit to the right index. Fix this
> using the bit manipulation macros.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_sandbox.c | 2 +-
>  test/dm/virtio.c                | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

I love the fix + test, thank you.

Re: [PATCH 07/11] test: dm: virtio: Test notify before del_vqs

[Simon Glass]

On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
>
> The virtqueue is passed to virtio_notify() so move the virtqueue
> deletion to the end of the test when it's no longer needed. This wasn't
> causing any problems because the sandbox virtio transport driver doesn't
> do anything for notifications, but it could cause problems if things
> change and it was a bad example.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  test/dm/virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 09/11] test: dm: virtio: Test virtio device driver probing

[Simon Glass]

On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
>
> Once the virtio-rng driver has been bound, probe it to trigger the pre
> and post child probe hooks of the virtio uclass driver. Check the status
> of the virtio device to confirm it reached the expected state.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  test/dm/virtio.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 11/11] RFC: test: dm: virtio: Test virtio-rng with faked device

[Andrew Scull]

On Mon, 11 Apr 2022 at 19:35, Simon Glass <sjg@chromium.org> wrote:
>
> On Thu, 31 Mar 2022 at 04:10, Andrew Scull <ascull@google.com> wrote:
> >
> > When looking into possibilities for testing virtio drivers I was trying
> > to simulate the device's responses in the virtqueue. It required a hack
> > to get access to the virtqueue by accessing the driver's private data
> > and only allows pre-programmed buffer returns but no dynamic responses,
> > data or descriptor modifications.
> >
> > This is an example of a regression test for the virtio-rng fix in the
> > previous patch.
> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >  test/dm/virtio.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Seems fine to me.

If using dev_get_priv() in the test is ok, the commit message should be updated:
..

test: dm: virtio: Test virtio-rng with faked device

Add a regression test for virtio-rng reading beyond the end of its
buffer if the virtio device provides an invalid length.

Signed-off-by: Andrew Scull <ascull@google.com>

Re: [PATCH 00/11] virtio: Harden and test vring

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

On Thu, Mar 31, 2022 at 10:09:38AM +0000, Andrew Scull wrote:

> Continuing the theme of making the virtio code resilient against
> corruption of the buffers shared with the device, this series focusses
> on the vring. This series is simpler and more self-contained than the
> series for virtio-pci!
> 
> It follows the example of Linux by keeping a private copy of the
> descriptors and metadata for state tracking and only ever writing to the
> descriptors that are shared with the device.
> 
> I was able to test these hardening steps in the sandbox by simulating
> device writes to the queues. I was also looking into testing the device
> drivers against a simulated device but the lack of an API to access the
> virtqueues meant this ended up being a hack. I've included that hack and
> the at the end of the series as an RFC.
> 
> Andrew Scull (11):
>   virtio_ring: Merge identical variables
>   virtio_ring: Add helper to attach vring descriptor
>   virtio_ring: Maintain a shadow copy of descriptors
>   virtio_ring: Check used descriptors are chain heads
>   dm: test: virtio: Test the virtio ring
>   virtio: sandbox: Fix device features bitfield
>   test: dm: virtio: Test notify before del_vqs
>   virtio: sandbox: Bind RNG rather than block device
>   test: dm: virtio: Test virtio device driver probing
>   virtio: rng: Check length before copying
>   RFC: test: dm: virtio: Test virtio-rng with faked device

What does this series depend on?  I got a failure to build on sandbox:
https://source.denx.de/u-boot/u-boot/-/jobs/422500#L104
Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/11] virtio: Harden and test vring

[Andrew Scull]

On Tue, 12 Apr 2022 at 19:10, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Mar 31, 2022 at 10:09:38AM +0000, Andrew Scull wrote:
>
> > Continuing the theme of making the virtio code resilient against
> > corruption of the buffers shared with the device, this series focusses
> > on the vring. This series is simpler and more self-contained than the
> > series for virtio-pci!
> >
> > It follows the example of Linux by keeping a private copy of the
> > descriptors and metadata for state tracking and only ever writing to the
> > descriptors that are shared with the device.
> >
> > I was able to test these hardening steps in the sandbox by simulating
> > device writes to the queues. I was also looking into testing the device
> > drivers against a simulated device but the lack of an API to access the
> > virtqueues meant this ended up being a hack. I've included that hack and
> > the at the end of the series as an RFC.
> >
> > Andrew Scull (11):
> >   virtio_ring: Merge identical variables
> >   virtio_ring: Add helper to attach vring descriptor
> >   virtio_ring: Maintain a shadow copy of descriptors
> >   virtio_ring: Check used descriptors are chain heads
> >   dm: test: virtio: Test the virtio ring
> >   virtio: sandbox: Fix device features bitfield
> >   test: dm: virtio: Test notify before del_vqs
> >   virtio: sandbox: Bind RNG rather than block device
> >   test: dm: virtio: Test virtio device driver probing
> >   virtio: rng: Check length before copying
> >   RFC: test: dm: virtio: Test virtio-rng with faked device
>
> What does this series depend on?  I got a failure to build on sandbox:
> https://source.denx.de/u-boot/u-boot/-/jobs/422500#L104

Problem is from the final, RFC, patch on SPL where CONFIG_DM_RNG is
not set so `dm_rng_read` isn't defined. I don't really understand the
difference with SPL just yet, but I expect CONFIG_DM_RNG can be set.
But in the meantime, it's also fine to drop that final patch from the
series.

Re: [PATCH 00/11] virtio: Harden and test vring

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]

On Tue, Apr 12, 2022 at 11:49:12PM +0100, Andrew Scull wrote:
> On Tue, 12 Apr 2022 at 19:10, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Mar 31, 2022 at 10:09:38AM +0000, Andrew Scull wrote:
> >
> > > Continuing the theme of making the virtio code resilient against
> > > corruption of the buffers shared with the device, this series focusses
> > > on the vring. This series is simpler and more self-contained than the
> > > series for virtio-pci!
> > >
> > > It follows the example of Linux by keeping a private copy of the
> > > descriptors and metadata for state tracking and only ever writing to the
> > > descriptors that are shared with the device.
> > >
> > > I was able to test these hardening steps in the sandbox by simulating
> > > device writes to the queues. I was also looking into testing the device
> > > drivers against a simulated device but the lack of an API to access the
> > > virtqueues meant this ended up being a hack. I've included that hack and
> > > the at the end of the series as an RFC.
> > >
> > > Andrew Scull (11):
> > >   virtio_ring: Merge identical variables
> > >   virtio_ring: Add helper to attach vring descriptor
> > >   virtio_ring: Maintain a shadow copy of descriptors
> > >   virtio_ring: Check used descriptors are chain heads
> > >   dm: test: virtio: Test the virtio ring
> > >   virtio: sandbox: Fix device features bitfield
> > >   test: dm: virtio: Test notify before del_vqs
> > >   virtio: sandbox: Bind RNG rather than block device
> > >   test: dm: virtio: Test virtio device driver probing
> > >   virtio: rng: Check length before copying
> > >   RFC: test: dm: virtio: Test virtio-rng with faked device
> >
> > What does this series depend on?  I got a failure to build on sandbox:
> > https://source.denx.de/u-boot/u-boot/-/jobs/422500#L104
> 
> Problem is from the final, RFC, patch on SPL where CONFIG_DM_RNG is
> not set so `dm_rng_read` isn't defined. I don't really understand the
> difference with SPL just yet, but I expect CONFIG_DM_RNG can be set.
> But in the meantime, it's also fine to drop that final patch from the
> series.

I'd like to have the test included.  SPL (and TPL) are special builds of
U-Boot used earlier on in the boot process so that we can load full
U-Boot, but have less resources and subsystems available.  Can you
please take a look at modifying the patch so that we only try and test
on full U-Boot?  There should be some other examples in the tests
directory doing what you need to do here.  Thanks!


-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]