[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1] virtio: add checks for the size of the indir
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [PATCH v1] virtio: add checks for the size of the indirect table |
Date: |
Tue, 15 Jan 2019 11:40:09 +0100 |
On Tue, 15 Jan 2019 13:08:47 +0300
Dima Stepanov <address@hidden> wrote:
> The virtqueue_pop() and virtqueue_get_avail_bytes() routines can use the
> INDIRECT table to get the data. It is possible to create a packet which
> will lead to the assert message like:
> include/exec/memory.h:1995: void
> address_space_read_cached(MemoryRegionCache *, hwaddr, void *, int):
> Assertion `addr < cache->len && len <= cache->len - addr' failed.
> Aborted
> To do it the first descriptor should have a link to the INDIRECT table
> and set the size of it to 0. It doesn't look good that the guest should
> be able to trigger the assert in qemu. Add additional check for the size
> of the INDIRECT table, which should not be 0.
Ouch, being able to crash QEMU by a specially crafted descriptor is bad.
Looking at the virtio spec, we don't seem to explicitly disallow
indirect descriptors with a zero-length table. So, as an alternative to
marking the device broken, we could also skip over such a descriptor.
Not sure whether that makes sense, though.
>
> Signed-off-by: Dima Stepanov <address@hidden>
> ---
> hw/virtio/virtio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 22bd1ac..a1ff647 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -646,7 +646,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned
> int *in_bytes,
> vring_desc_read(vdev, &desc, desc_cache, i);
>
> if (desc.flags & VRING_DESC_F_INDIRECT) {
> - if (desc.len % sizeof(VRingDesc)) {
> + if (!desc.len || (desc.len % sizeof(VRingDesc))) {
> virtio_error(vdev, "Invalid size for indirect buffer table");
> goto err;
> }
> @@ -902,7 +902,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
> desc_cache = &caches->desc;
> vring_desc_read(vdev, &desc, desc_cache, i);
> if (desc.flags & VRING_DESC_F_INDIRECT) {
> - if (desc.len % sizeof(VRingDesc)) {
> + if (!desc.len || (desc.len % sizeof(VRingDesc))) {
> virtio_error(vdev, "Invalid size for indirect buffer table");
> goto done;
> }