On Mon, 11 Apr 2022 at 19:36, Simon Glass sjg@chromium.org wrote:
Hi Andrew,
On Thu, 7 Apr 2022 at 03:41, Andrew Scull ascull@google.com wrote:
Add a fuzzer to test the vring handling code against unexpected mutations from the virtio device.
After building the sandbox with CONFIG_FUZZ=y, the fuzzer can be invoked with by:
UBOOT_SB_FUZZ_TEST=fuzz_vring ./u-boot
This fuzzer finds unvalidated inputs in the vring driver that allow a buggy or malicious device to make the driver chase wild pointers.
Signed-off-by: Andrew Scull ascull@google.com
test/fuzz/Makefile | 1 + test/fuzz/virtio.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) create mode 100644 test/fuzz/virtio.c
diff --git a/test/fuzz/Makefile b/test/fuzz/Makefile index 03eeeeb497..663b79ce80 100644 --- a/test/fuzz/Makefile +++ b/test/fuzz/Makefile @@ -5,3 +5,4 @@ #
obj-$(CONFIG_$(SPL_)CMDLINE) += cmd_fuzz.o +obj-$(CONFIG_VIRTIO_SANDBOX) += virtio.o diff --git a/test/fuzz/virtio.c b/test/fuzz/virtio.c new file mode 100644 index 0000000000..e5363d5638 --- /dev/null +++ b/test/fuzz/virtio.c @@ -0,0 +1,72 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Copyright (c) 2022 Google, Inc.
- Written by Andrew Scull ascull@google.com
- */
+#include <common.h> +#include <dm.h> +#include <virtio.h> +#include <virtio_ring.h> +#include <test/fuzz.h>
+static int fuzz_vring(const uint8_t *data, size_t size) +{
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];/* hackily hardcode vring sizes */size_t num = 4;size_t desc_size = (sizeof(struct vring_desc) * num);size_t avail_size = (3 + num) * sizeof(u16);size_t used_size = (3 * sizeof(u16)) + (sizeof(struct vring_used_elem) * num);if (size < (desc_size + avail_size + used_size))return 0;/* check probe success */if (uclass_first_device(UCLASS_VIRTIO, &bus) || !bus)panic("Could not find virtio bus\n");Do we have to panic? Could this use some sort of assert function, or similar? It seems like we should try to show the error then return an error code from this function, rather than just panic in the middle. We could have something like the ut_assert... stuff.
Would a basic assert() be better rather than handling with panic()? We should never see this fail, and if it does we want something quite dramatic to happen so that the fuzzer notices and tells us something went wrong.
/* check the child virtio-rng device is bound */if (device_find_first_child(bus, &dev) || !dev)panic("Could not find virtio device\n");/** 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);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];if (virtio_find_vqs(dev, 1, &vq))panic("Could not find vqs\n");if (virtqueue_add(vq, sgs, 0, 1))panic("Could not add to virtqueue\n");/* Simulate device writing to vring */memcpy(vq->vring.desc, data, desc_size);memcpy(vq->vring.avail, data + desc_size, avail_size);memcpy(vq->vring.used, data + desc_size + avail_size, used_size);/* Make sure there is a response */if (vq->vring.used->idx == 0)vq->vring.used->idx = 1;virtqueue_get_buf(vq, &len);if (virtio_del_vqs(dev))panic("Could not delete vqs\n");return 0;+}
+FUZZ_TEST(fuzz_vring, 0);
2.35.1.1094.g7c7d902a7c-goog
Regards, Simon