Re: [Qemu-devel] [PATCH v1] virtio: add checks for the size of the indirect table

[Dima Stepanov]

On Tue, Jan 15, 2019 at 11:40:09AM +0100, Cornelia Huck wrote:
> 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.

Hard to say what is the better option here: mark device with the
VIRTIO_CONFIG_S_NEEDS_RESET bit or just skip the descriptor. Right now
all the parsing errors are handled using the virtio_error() call. The
possible parsing errors are: wrong address, looped descriptors, invalid
size, incorrect order and so on. Some of those errors are not described
in the virtio spec. So it looks like that this error should be also
handled by calling virtio_error().

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