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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jan 15, 2019 at 02:38:22PM +0100, Cornelia Huck wrote:
> On Tue, 15 Jan 2019 16:11:19 +0300
> Dima Stepanov <dimastep@yandex-team.ru> wrote:
> 
> > On Tue, Jan 15, 2019 at 11:40:09AM +0100, Cornelia Huck wrote:
> > > On Tue, 15 Jan 2019 13:08:47 +0300
> > > Dima Stepanov <dimastep@yandex-team.ru> 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.

Well guest root can easily just halt so I'm not too concerned about it.

> > > 
> > > 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().
> 
> virtio_error() is certainly the safe option (and easiest to implement),
> and handling weird descriptors is probably not worth the time.
> 
> FWIW,
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> Should this be cc:stable, as it is a guest-triggerable crash?

Sure, why not.

> > 
> > >   
> > > > 
> > > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> > > > ---
> > > >  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;
> > > >          }  
> > >