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

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

[Dima Stepanov]

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.

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

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

[Philippe Mathieu-Daudé]

On 1/15/19 11:08 AM, Dima Stepanov 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.
> 
> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

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

[Cornelia Huck]

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.

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

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

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

[Cornelia Huck]

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.
> > 
> > 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?

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

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

[Stefan Hajnoczi]

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

On Tue, Jan 15, 2019 at 01:08:47PM +0300, Dima Stepanov 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.
> 
> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> ---
>  hw/virtio/virtio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[Michael S. Tsirkin]

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