[Qemu-devel] [PATCH 1/2] libvhost-user: fix vu_queue_get_avail_bytes() crash

[Qemu-devel] [PATCH 1/2] libvhost-user: fix vu_queue_get_avail_bytes() crash

[Marc-André Lureau]

Calling vu_queue_get_avail_bytes() when the queue doesn't yet have
addresses will result in the following crash:

Program received signal SIGSEGV, Segmentation fault.
0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68, vq=0x55c41582fd68)
    at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
940            vq->shadow_avail_idx = vq->vring.avail->idx;
(gdb) p vq
$1 = (VuVirtq *) 0x55c41582fd68
(gdb) p vq->vring
$2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0, flags = 0}

    at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
No locals.
    at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960
        num_heads = <optimized out>
    out_bytes=out_bytes@entry=0x7fffd035d7c4, max_in_bytes=max_in_bytes@entry=0,
    max_out_bytes=max_out_bytes@entry=0) at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034

Check if vring.avail != null before accessing it.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 contrib/libvhost-user/libvhost-user.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index af4faad60b..f9680b6279 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int *in_bytes,
     idx = vq->last_avail_idx;
 
     total_bufs = in_total = out_total = 0;
+    if (!vq->vring.avail) {
+        goto done;
+    }
+
     while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) {
         unsigned int max, num_bufs, indirect = 0;
         struct vring_desc *desc;
-- 
2.12.0.191.gc5d8de91d

[Qemu-devel] [PATCH 2/2] libvhost-user: make vu_queue_empty() safer

[Marc-André Lureau]

Check if vring.avail != null before accessing it.

Fix crashes from callers such a vu_queue_pop() when queue isn't ready.

Fix documentation and return type while at it.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 contrib/libvhost-user/libvhost-user.h | 6 +++---
 contrib/libvhost-user/libvhost-user.c | 8 ++++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h
index 156b50e989..af02a31ebe 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -327,13 +327,13 @@ void vu_queue_set_notification(VuDev *dev, VuVirtq *vq, int enable);
 bool vu_queue_enabled(VuDev *dev, VuVirtq *vq);
 
 /**
- * vu_queue_enabled:
+ * vu_queue_empty:
  * @dev: a VuDev context
  * @vq: a VuVirtq queue
  *
- * Returns: whether the queue is empty.
+ * Returns: true if the queue is empty or not ready.
  */
-int vu_queue_empty(VuDev *dev, VuVirtq *vq);
+bool vu_queue_empty(VuDev *dev, VuVirtq *vq);
 
 /**
  * vu_queue_notify:
diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index f9680b6279..b0f15b76e3 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -1125,11 +1125,15 @@ vu_queue_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int in_bytes,
 
 /* Fetch avail_idx from VQ memory only when we really need to know if
  * guest has added some buffers. */
-int
+bool
 vu_queue_empty(VuDev *dev, VuVirtq *vq)
 {
+    if (!vq->vring.avail) {
+        return true;
+    }
+
     if (vq->shadow_avail_idx != vq->last_avail_idx) {
-        return 0;
+        return false;
     }
 
     return vring_avail_idx(vq) == vq->last_avail_idx;
-- 
2.12.0.191.gc5d8de91d

Re: [Qemu-devel] [PATCH 1/2] libvhost-user: fix vu_queue_get_avail_bytes() crash

[Philippe Mathieu-Daudé]

Hi Marc-André,

On 05/03/2017 09:11 AM, Marc-André Lureau wrote:
> Calling vu_queue_get_avail_bytes() when the queue doesn't yet have
> addresses will result in the following crash:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68, vq=0x55c41582fd68)
>     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
> 940            vq->shadow_avail_idx = vq->vring.avail->idx;
> (gdb) p vq
> $1 = (VuVirtq *) 0x55c41582fd68
> (gdb) p vq->vring
> $2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0, flags = 0}
>
>     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
> No locals.
>     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960
>         num_heads = <optimized out>
>     out_bytes=out_bytes@entry=0x7fffd035d7c4, max_in_bytes=max_in_bytes@entry=0,
>     max_out_bytes=max_out_bytes@entry=0) at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034
>
> Check if vring.avail != null before accessing it.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  contrib/libvhost-user/libvhost-user.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> index af4faad60b..f9680b6279 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int *in_bytes,
>      idx = vq->last_avail_idx;
>
>      total_bufs = in_total = out_total = 0;
> +    if (!vq->vring.avail) {
> +        goto done;
> +    }
> +
>      while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) {
>          unsigned int max, num_bufs, indirect = 0;
>          struct vring_desc *desc;
>

It seems to me safer to fix instead vring_avail_ring(), and fix 
neighbours vring_avail_flags() and vring_avail_idx() while here.

Re: [Qemu-devel] [PATCH 2/2] libvhost-user: make vu_queue_empty() safer

[Philippe Mathieu-Daudé]

On 05/03/2017 09:11 AM, Marc-André Lureau wrote:
> Check if vring.avail != null before accessing it.
>
> Fix crashes from callers such a vu_queue_pop() when queue isn't ready.
>
> Fix documentation and return type while at it.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  contrib/libvhost-user/libvhost-user.h | 6 +++---
>  contrib/libvhost-user/libvhost-user.c | 8 ++++++--
>  2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h
> index 156b50e989..af02a31ebe 100644
> --- a/contrib/libvhost-user/libvhost-user.h
> +++ b/contrib/libvhost-user/libvhost-user.h
> @@ -327,13 +327,13 @@ void vu_queue_set_notification(VuDev *dev, VuVirtq *vq, int enable);
>  bool vu_queue_enabled(VuDev *dev, VuVirtq *vq);
>
>  /**
> - * vu_queue_enabled:
> + * vu_queue_empty:
>   * @dev: a VuDev context
>   * @vq: a VuVirtq queue
>   *
> - * Returns: whether the queue is empty.
> + * Returns: true if the queue is empty or not ready.
>   */
> -int vu_queue_empty(VuDev *dev, VuVirtq *vq);
> +bool vu_queue_empty(VuDev *dev, VuVirtq *vq);
>
>  /**
>   * vu_queue_notify:
> diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> index f9680b6279..b0f15b76e3 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -1125,11 +1125,15 @@ vu_queue_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int in_bytes,
>
>  /* Fetch avail_idx from VQ memory only when we really need to know if
>   * guest has added some buffers. */
> -int
> +bool
>  vu_queue_empty(VuDev *dev, VuVirtq *vq)
>  {
> +    if (!vq->vring.avail) {
> +        return true;
> +    }
> +
>      if (vq->shadow_avail_idx != vq->last_avail_idx) {
> -        return 0;
> +        return false;
>      }
>
>      return vring_avail_idx(vq) == vq->last_avail_idx;
>

Re: [Qemu-devel] [PATCH 1/2] libvhost-user: fix vu_queue_get_avail_bytes() crash

[Marc-André Lureau]

Hi

----- Original Message -----
> Hi Marc-André,
> 
> On 05/03/2017 09:11 AM, Marc-André Lureau wrote:
> > Calling vu_queue_get_avail_bytes() when the queue doesn't yet have
> > addresses will result in the following crash:
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68,
> > vq=0x55c41582fd68)
> >     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
> > 940            vq->shadow_avail_idx = vq->vring.avail->idx;
> > (gdb) p vq
> > $1 = (VuVirtq *) 0x55c41582fd68
> > (gdb) p vq->vring
> > $2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0,
> > flags = 0}
> >
> >     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
> > No locals.
> >     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960
> >         num_heads = <optimized out>
> >     out_bytes=out_bytes@entry=0x7fffd035d7c4,
> >     max_in_bytes=max_in_bytes@entry=0,
> >     max_out_bytes=max_out_bytes@entry=0) at
> >     /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034
> >
> > Check if vring.avail != null before accessing it.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  contrib/libvhost-user/libvhost-user.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/contrib/libvhost-user/libvhost-user.c
> > b/contrib/libvhost-user/libvhost-user.c
> > index af4faad60b..f9680b6279 100644
> > --- a/contrib/libvhost-user/libvhost-user.c
> > +++ b/contrib/libvhost-user/libvhost-user.c
> > @@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq,
> > unsigned int *in_bytes,
> >      idx = vq->last_avail_idx;
> >
> >      total_bufs = in_total = out_total = 0;
> > +    if (!vq->vring.avail) {
> > +        goto done;
> > +    }
> > +
> >      while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) {
> >          unsigned int max, num_bufs, indirect = 0;
> >          struct vring_desc *desc;
> >
> 
> It seems to me safer to fix instead vring_avail_ring(), and fix
> neighbours vring_avail_flags() and vring_avail_idx() while here.

Those are internal/static functions, possibly on hot paths. So I would rather keep the check on the external/public functions only.

Re: [Qemu-devel] [PATCH 1/2] libvhost-user: fix vu_queue_get_avail_bytes() crash

[Philippe Mathieu-Daudé]

>> On 05/03/2017 09:11 AM, Marc-André Lureau wrote:
>>> Calling vu_queue_get_avail_bytes() when the queue doesn't yet have
>>> addresses will result in the following crash:
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> 0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68,
>>> vq=0x55c41582fd68)
>>>     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
>>> 940            vq->shadow_avail_idx = vq->vring.avail->idx;
>>> (gdb) p vq
>>> $1 = (VuVirtq *) 0x55c41582fd68
>>> (gdb) p vq->vring
>>> $2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0,
>>> flags = 0}
>>>
>>>     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
>>> No locals.
>>>     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960
>>>         num_heads = <optimized out>
>>>     out_bytes=out_bytes@entry=0x7fffd035d7c4,
>>>     max_in_bytes=max_in_bytes@entry=0,
>>>     max_out_bytes=max_out_bytes@entry=0) at
>>>     /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034
>>>
>>> Check if vring.avail != null before accessing it.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  contrib/libvhost-user/libvhost-user.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/contrib/libvhost-user/libvhost-user.c
>>> b/contrib/libvhost-user/libvhost-user.c
>>> index af4faad60b..f9680b6279 100644
>>> --- a/contrib/libvhost-user/libvhost-user.c
>>> +++ b/contrib/libvhost-user/libvhost-user.c
>>> @@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq,
>>> unsigned int *in_bytes,
>>>      idx = vq->last_avail_idx;
>>>
>>>      total_bufs = in_total = out_total = 0;
>>> +    if (!vq->vring.avail) {
>>> +        goto done;
>>> +    }
>>> +
>>>      while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) {
>>>          unsigned int max, num_bufs, indirect = 0;
>>>          struct vring_desc *desc;
>>>
>>
>> It seems to me safer to fix instead vring_avail_ring(), and fix
>> neighbours vring_avail_flags() and vring_avail_idx() while here.
>
> Those are internal/static functions, possibly on hot paths. So I would rather keep the check on the external/public functions only.

Fair enough.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH 1/2] libvhost-user: fix vu_queue_get_avail_bytes() crash

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> Calling vu_queue_get_avail_bytes() when the queue doesn't yet have
> addresses will result in the following crash:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68, vq=0x55c41582fd68)
>     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
> 940            vq->shadow_avail_idx = vq->vring.avail->idx;
> (gdb) p vq
> $1 = (VuVirtq *) 0x55c41582fd68
> (gdb) p vq->vring
> $2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0, flags = 0}
> 
>     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940
> No locals.
>     at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960
>         num_heads = <optimized out>
>     out_bytes=out_bytes@entry=0x7fffd035d7c4, max_in_bytes=max_in_bytes@entry=0,
>     max_out_bytes=max_out_bytes@entry=0) at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034
> 
> Check if vring.avail != null before accessing it.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>


This doesn't seem to fully solve it, it's now failing at:

#0  vring_used_idx_set (val=0, vq=0x5570fcb49d68, dev=0x5570fcb49c20)
    at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1472
No locals.
#1  vu_queue_flush (dev=0x5570fcb49c20, vq=0x5570fcb49d68, count=0)
    at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1494
        old = 0
        new = 0
#2  0x00005570fb8d9639 in vubr_backend_recv_cb (sock=<optimized out>, ctx=0x5570fcb49c20)
    at /home/dgilbert/git/qemu/tests/vhost-user-bridge.c:349
        vubr = 0x5570fcb49c20
        dev = 0x5570fcb49c20
        vq = 0x5570fcb49d68
        elem = <optimized out>
        mhdr_sg = {{iov_base = 0x0, iov_len = 0} <repeats 740 times>, {iov_base = 0x0, iov_len = 139877843420656}, {

            iov_len = 0}...}
        mhdr = {hdr = {flags = 0 '\000', gso_type = 0 '\000', hdr_len = 0, gso_size = 0, csum_start = 0, 
            csum_offset = 0}, num_buffers = 0}
        mhdr_cnt = <optimized out>
        hdrlen = 0
        i = <optimized out>
        hdr = {flags = 0 '\000', gso_type = 0 '\000', hdr_len = 0, gso_size = 0, csum_start = 0, csum_offset = 0}
        __PRETTY_FUNCTION__ = "vubr_backend_recv_cb"
#3  0x00005570fb8d8ad3 in dispatcher_wait (timeout=200000, dispr=0x5570fcb4a0b8)
    at /home/dgilbert/git/qemu/tests/vhost-user-bridge.c:154
        e = 0x5570fcb4a180
        tv = {tv_sec = 0, tv_usec = 199998}
        fdset = {fds_bits = {48, 0 <repeats 15 times>}}
        rc = <optimized out>
        sock = 4
#4  vubr_run (dev=<optimized out>) at /home/dgilbert/git/qemu/tests/vhost-user-bridge.c:624
No locals.
#5  main (argc=<optimized out>, argv=<optimized out>) at /home/dgilbert/git/qemu/tests/vhost-user-bridge.c:697
        opt = <optimized out>
        client = <optimized out>

Dave

> ---
>  contrib/libvhost-user/libvhost-user.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> index af4faad60b..f9680b6279 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int *in_bytes,
>      idx = vq->last_avail_idx;
>  
>      total_bufs = in_total = out_total = 0;
> +    if (!vq->vring.avail) {
> +        goto done;
> +    }
> +
>      while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) {
>          unsigned int max, num_bufs, indirect = 0;
>          struct vring_desc *desc;
> -- 
> 2.12.0.191.gc5d8de91d
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK