[v3,1/2] virtio: check VirtQueue Vring object is set

From: Prasad J Pandit <pjp@fedoraproject.org>

An user could attempt to use an uninitialised VirtQueue object
or unset Vring.align leading to a arithmetic exception. Add check
to avoid it.

Reported-by: Zhangboxian <zhangboxian@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/virtio/virtio.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

On Sat, 25 Nov 2017 00:04:45 +0530
P J P <ppandit@redhat.com> wrote:

> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> An user could attempt to use an uninitialised VirtQueue object

s/An user/A guest/ ?

> or unset Vring.align leading to a arithmetic exception. Add check
> to avoid it.
> 
> Reported-by: Zhangboxian <zhangboxian@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/virtio/virtio.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5884ce3480..c01eac87a5 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -182,7 +182,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n)
>  {
>      VRing *vring = &vdev->vq[n].vring;
>  
> -    if (!vring->desc) {
> +    if (!vdev->vq[n].vring.num || !vring->desc || !vring->align) {
>          /* not yet setup -> nothing to do */
>          return;
>      }
> @@ -1414,6 +1414,9 @@ void virtio_config_modern_writel(VirtIODevice *vdev,
>  
>  void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr)
>  {
> +    if (!vdev->vq[n].vring.num) {
> +        return;
> +    }
>      vdev->vq[n].vring.desc = addr;
>      virtio_queue_update_rings(vdev, n);
>  }
> @@ -1426,6 +1429,9 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
>  void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
>                              hwaddr avail, hwaddr used)
>  {
> +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {


Checking for !desc is wrong (why shouldn't a driver be able to unset a
descriptor table?)

The check for align is not really needed, as virtio-1 disallows setting
align anyway.

> +        return;
> +    }
>      vdev->vq[n].vring.desc = desc;
>      vdev->vq[n].vring.avail = avail;
>      vdev->vq[n].vring.used = used;
> @@ -1494,8 +1500,10 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
>       */
>      assert(k->has_variable_vring_alignment);
>  
> -    vdev->vq[n].vring.align = align;
> -    virtio_queue_update_rings(vdev, n);
> +    if (align) {
> +        vdev->vq[n].vring.align = align;
> +        virtio_queue_update_rings(vdev, n);
> +    }
>  }
>  
>  static bool virtio_queue_notify_aio_vq(VirtQueue *vq)

On Sat, Nov 25, 2017 at 12:04:45AM +0530, P J P wrote:
> @@ -1426,6 +1429,9 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
>  void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
>                              hwaddr avail, hwaddr used)
>  {
> +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {

Why !desc?

+-- On Mon, 27 Nov 2017, Cornelia Huck wrote --+
|The check for align is not really needed, as virtio-1 disallows setting align 
|anyway.

 disallows...?

| Checking for !desc is wrong (why shouldn't a driver be able to unset a
| descriptor table?)

+-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+
| > +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {
| ...
|   vdev->vq[n].vring.desc = desc;
|
| Why !desc?

virtio_queue_set_rings
 virtio_init_region_cache
   VirtQueue *vq = &vdev->vq[n];
   ...
   addr = vq->vring.desc;
   if (!addr) {
       return;
   }

These checks seem to be repeating all over. As mentioned earlier, could these 
be collated in one place, maybe virtio_queue_get_num()?

  int virtio_queue_get_num(VirtIODevice *vdev, int n)
  {
      VirtQueue *vq = &vdev->vq[n];

      if (!vq->.vring.num
           || !vq->vring.desc
           || !vq->vring.align) {
          return 0;  /* vq not set */
      }

      return vdev->vq[n].vring.num;
  }


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

On Mon, 27 Nov 2017 23:25:28 +0530 (IST)
P J P <ppandit@redhat.com> wrote:

> +-- On Mon, 27 Nov 2017, Cornelia Huck wrote --+
> |The check for align is not really needed, as virtio-1 disallows setting align 
> |anyway.
> 
>  disallows...?

See the check in virtio_queue_set_align(). Moreover, the calculation
that breaks virtqueue setup for align == 0 is only called for the
legacy setup, IOW not for this virtio-1 only function.

> 
> | Checking for !desc is wrong (why shouldn't a driver be able to unset a
> | descriptor table?)
> 
> +-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+
> | > +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {
> | ...
> |   vdev->vq[n].vring.desc = desc;
> |
> | Why !desc?
> 
> virtio_queue_set_rings
>  virtio_init_region_cache
>    VirtQueue *vq = &vdev->vq[n];
>    ...
>    addr = vq->vring.desc;
>    if (!addr) {
>        return;
>    }
> 
> These checks seem to be repeating all over. As mentioned earlier, could these 
> be collated in one place, maybe virtio_queue_get_num()?
> 
>   int virtio_queue_get_num(VirtIODevice *vdev, int n)
>   {
>       VirtQueue *vq = &vdev->vq[n];
> 
>       if (!vq->.vring.num
>            || !vq->vring.desc
>            || !vq->vring.align) {
>           return 0;  /* vq not set */
>       }
> 
>       return vdev->vq[n].vring.num;
>   }

This is conflating different things:
- vq does not exist (num == 0)
- vq is not setup by the guest (desc == 0)
- vq has no valid alignment (which is only relevant for legacy)

On Tue, Nov 28, 2017 at 10:11:54AM +0100, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 23:25:28 +0530 (IST)
> P J P <ppandit@redhat.com> wrote:
> > +-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+
> > | > +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {
> > | ...
> > |   vdev->vq[n].vring.desc = desc;
> > |
> > | Why !desc?
> > 
> > virtio_queue_set_rings
> >  virtio_init_region_cache
> >    VirtQueue *vq = &vdev->vq[n];
> >    ...
> >    addr = vq->vring.desc;
> >    if (!addr) {
> >        return;
> >    }
> > 
> > These checks seem to be repeating all over. As mentioned earlier, could these 
> > be collated in one place, maybe virtio_queue_get_num()?
> > 
> >   int virtio_queue_get_num(VirtIODevice *vdev, int n)
> >   {
> >       VirtQueue *vq = &vdev->vq[n];
> > 
> >       if (!vq->.vring.num
> >            || !vq->vring.desc
> >            || !vq->vring.align) {
> >           return 0;  /* vq not set */
> >       }
> > 
> >       return vdev->vq[n].vring.num;
> >   }
> 
> This is conflating different things:
> - vq does not exist (num == 0)
> - vq is not setup by the guest (desc == 0)
> - vq has no valid alignment (which is only relevant for legacy)

I agree.

Stefan

+-- On Tue, 28 Nov 2017, Stefan Hajnoczi wrote --+
| > This is conflating different things:
| > - vq does not exist (num == 0)
| > - vq is not setup by the guest (desc == 0)
| > - vq has no valid alignment (which is only relevant for legacy)
| 
| I agree.

Either case, vq would be unfit for use, no?

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

On Tue, 28 Nov 2017 16:57:34 +0530 (IST)
P J P <ppandit@redhat.com> wrote:

> +-- On Tue, 28 Nov 2017, Stefan Hajnoczi wrote --+
> | > This is conflating different things:
> | > - vq does not exist (num == 0)
> | > - vq is not setup by the guest (desc == 0)
> | > - vq has no valid alignment (which is only relevant for legacy)
> | 
> | I agree.
> 
> Either case, vq would be unfit for use, no?

What is "unfit for use"?

I'm not quite sure what you want to achieve with this patch. I assume
you want to fix the issue that a guest may provide invalid values for
align etc. which can cause qemu to crash or behave badly.

If so, you need to do different things for the different points above.
- The guest should not muck around with a non-existing queue (num == 0)
  in any case, so this should be fenced for any manipulation triggered
  by the guest.
- Processing a non-setup queue (desc == 0; also applies to the other
  buffers for virtio-1) should be skipped. However, _setting_ desc etc.
  to 0 from the guest is fine (as long as it follows the other
  constraints of the spec).
- Setting alignment to 0 only applies to legacy + virtio-mmio. I would
  not overengineer fencing this. A simple check in update_rings should
  be enough.

Hello Cornelia,

+-- On Tue, 28 Nov 2017, Cornelia Huck wrote --+
| What is "unfit for use"?

Unfit for use because we see checks like

  if (!virtio_queue_get_num(vdev, n)) {
            continue;
  ...
  if (!vdev->vq[n].vring.num) {
      return;

'virtio_queue_set_rings' sets 'vring.desc' as

  vdev->vq[n].vring.desc = desc;

and calls virtio_init_region_cache(vdev, n);
which returns if vq->vring.desc is zero(0).

  addr = vq->vring.desc;
  if (!addr) {
      return;
  }

Same in virtio_queue_set_addr() -> virtio_queue_update_rings().

It seems that for 'vq' instance to be useful, vring.num, vring.desc etc. 
fields need to be set properly. Unless an unused/free 'vq' is being accessed 
to set these fields.

| I'm not quite sure what you want to achieve with this patch. I assume
| you want to fix the issue that a guest may provide invalid values for
| align etc. which can cause qemu to crash or behave badly.

True. In the process I'm trying to figure out if a usable 'vq' instance could 
be decided in once place, than having repeating checks, if possible.

Ex. 'virtio_queue_update_rings' is called as

virtio_queue_set_addr
 -> virtio_queue_update_rings

virtio_queue_set_align
 -> virtio_queue_update_rings

virtio_load
 for (i = 0; i < num; i++) {
   if (vdev->vq[i].vring.desc) {
   ...
     virtio_queue_update_rings

Of these, virtio_load checks that 'vring.desc' is non-zero(0). Current 
patch adds couple checks to the other two callers above. And again,

virtio_queue_update_rings would check

    if (!vring->num || !vring->desc || !vring->align) {
       /* not yet setup -> nothing to do */
       return;
    }

| If so, you need to do different things for the different points above.
| - The guest should not muck around with a non-existing queue (num == 0)
|   in any case, so this should be fenced for any manipulation triggered
|   by the guest.

I guess done by !virtio_queue_get_num() check above?

| - Processing a non-setup queue (desc == 0; also applies to the other
|   buffers for virtio-1) should be skipped. However, _setting_ desc etc.
|   to 0 from the guest is fine (as long as it follows the other
|   constraints of the spec).

Okay. Though its non-zero(0) value is preferred?

| - Setting alignment to 0 only applies to legacy + virtio-mmio. I would
|   not overengineer fencing this. A simple check in update_rings should
|   be enough.

Okay.x


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

On Wed, 29 Nov 2017 15:41:45 +0530 (IST)
P J P <ppandit@redhat.com> wrote:

>   Hello Cornelia,
> 
> +-- On Tue, 28 Nov 2017, Cornelia Huck wrote --+
> | What is "unfit for use"?
> 
> Unfit for use because we see checks like
> 
>   if (!virtio_queue_get_num(vdev, n)) {
>             continue;
>   ...
>   if (!vdev->vq[n].vring.num) {
>       return;
> 
> 'virtio_queue_set_rings' sets 'vring.desc' as
> 
>   vdev->vq[n].vring.desc = desc;
> 
> and calls virtio_init_region_cache(vdev, n);
> which returns if vq->vring.desc is zero(0).
> 
>   addr = vq->vring.desc;
>   if (!addr) {
>       return;
>   }
> 
> Same in virtio_queue_set_addr() -> virtio_queue_update_rings().
> 
> It seems that for 'vq' instance to be useful, vring.num, vring.desc etc. 
> fields need to be set properly. Unless an unused/free 'vq' is being accessed 
> to set these fields.

I think the basic problem is still that you conflate two things:
- vring.num, which cannot be flipped between 0 and !0 by the guest
- vring.{desc,avail,used}, which can

IOW, if vring.num == 0, the guest cannot manipulate the queue; if
vring.desc == 0, the guest can. 

> 
> | I'm not quite sure what you want to achieve with this patch. I assume
> | you want to fix the issue that a guest may provide invalid values for
> | align etc. which can cause qemu to crash or behave badly.
> 
> True. In the process I'm trying to figure out if a usable 'vq' instance could 
> be decided in once place, than having repeating checks, if possible.
> 
> Ex. 'virtio_queue_update_rings' is called as
> 
> virtio_queue_set_addr
>  -> virtio_queue_update_rings  
> 
> virtio_queue_set_align
>  -> virtio_queue_update_rings  
> 
> virtio_load
>  for (i = 0; i < num; i++) {
>    if (vdev->vq[i].vring.desc) {
>    ...
>      virtio_queue_update_rings
> 
> Of these, virtio_load checks that 'vring.desc' is non-zero(0). Current 
> patch adds couple checks to the other two callers above. And again,
> 
> virtio_queue_update_rings would check
> 
>     if (!vring->num || !vring->desc || !vring->align) {
>        /* not yet setup -> nothing to do */
>        return;
>     }

vring.num and vring.desc are really different things. You don't want
the guest to do anything with the queue if vring.num == 0, while you
just want to skip various processing if vring.desc == 0.

(virtio_load() does not need to care about vring.num, as it is not
triggered by the guest.)

> 
> | If so, you need to do different things for the different points above.
> | - The guest should not muck around with a non-existing queue (num == 0)
> |   in any case, so this should be fenced for any manipulation triggered
> |   by the guest.
> 
> I guess done by !virtio_queue_get_num() check above?

Yes.

> 
> | - Processing a non-setup queue (desc == 0; also applies to the other
> |   buffers for virtio-1) should be skipped. However, _setting_ desc etc.
> |   to 0 from the guest is fine (as long as it follows the other
> |   constraints of the spec).
> 
> Okay. Though its non-zero(0) value is preferred?

Many functions have a likely/unlikely check, setup routines excepted.

> 
> | - Setting alignment to 0 only applies to legacy + virtio-mmio. I would
> |   not overengineer fencing this. A simple check in update_rings should
> |   be enough.
> 
> Okay.x

+-- On Wed, 29 Nov 2017, Cornelia Huck wrote --+
| I think the basic problem is still that you conflate two things:
| - vring.num, which cannot be flipped between 0 and !0 by the guest
| - vring.{desc,avail,used}, which can
| 
| IOW, if vring.num == 0, the guest cannot manipulate the queue; if
| vring.desc == 0, the guest can. 
| 
| Many functions have a likely/unlikely check, setup routines excepted.

Have sent a revised patch v4. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F