Libguestfs - Dec 2017 - [nbdkit PATCH] nbd: Fix sporadic use-after-free

Now that we properly clean up 'trans' in the reader thread, we
must not dereference 'trans' from the write thread at any point
after trans has been added to the list unless we have grabbed
it back off the list ourselves, or we risk an occasional
use-after-free or even double free that valgrind can detect.

Reported-by: Richard W.M. Jones <rjones@redhat.com>
Fixes: cb189648f11df6c4f7287dcaed4bc856650e2c3b
Signed-off-by: Eric Blake <eblake@redhat.com>
---

Perhaps I should also add more comments to the code about transfer of
ownership of 'trans' between threads?

 plugins/nbd/nbd.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index e79042c..9d40e87 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -227,6 +227,26 @@ nbd_mark_dead (struct handle *h)
   return -1;
 }

+/* Find and remove the transaction corresponding to cookie from the list. */
+static struct transaction *
+find_trans_by_cookie (struct handle *h, uint64_t cookie)
+{
+  struct transaction **ptr;
+  struct transaction *trans;
+
+  nbd_lock (h);
+  ptr = &h->trans;
+  while ((trans = *ptr) != NULL) {
+    if (cookie == trans->u.cookie)
+      break;
+    ptr = &trans->next;
+  }
+  if (trans)
+    *ptr = trans->next;
+  nbd_unlock (h);
+  return trans;
+}
+
 /* Send a request, return 0 on success or -1 on write failure. */
 static int
 nbd_request_raw (struct handle *h, uint32_t type, uint64_t offset,
@@ -260,6 +280,8 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t
offset,
 {
   int err;
   struct transaction *trans;
+  int fd;
+  uint64_t cookie;

   trans = calloc (1, sizeof *trans);
   if (!trans) {
@@ -282,9 +304,14 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t
offset,
   }
   trans->next = h->trans;
   h->trans = trans;
+  fd = trans->u.fds[0];
+  cookie = trans->u.cookie;
   nbd_unlock (h);
-  if (nbd_request_raw (h, type, offset, count, trans->u.cookie, req_buf) ==
0)
-    return trans->u.fds[0];
+  if (nbd_request_raw (h, type, offset, count, cookie, req_buf) == 0)
+    return fd;
+  trans = find_trans_by_cookie (h, cookie);
+  if (!trans)
+    return nbd_mark_dead (h);

  err:
   err = errno;
@@ -309,7 +336,6 @@ static int
 nbd_reply_raw (struct handle *h, int *fd)
 {
   struct reply rep;
-  struct transaction **ptr;
   struct transaction *trans;
   void *buf;
   uint32_t count;
@@ -320,16 +346,7 @@ nbd_reply_raw (struct handle *h, int *fd)
   if (be32toh (rep.magic) != NBD_REPLY_MAGIC)
     return nbd_mark_dead (h);
   nbdkit_debug ("received reply for cookie %#" PRIx64, rep.handle);
-  nbd_lock (h);
-  ptr = &h->trans;
-  while ((trans = *ptr) != NULL) {
-    if (rep.handle == trans->u.cookie)
-      break;
-    ptr = &trans->next;
-  }
-  if (trans)
-    *ptr = trans->next;
-  nbd_unlock (h);
+  trans = find_trans_by_cookie (h, rep.handle);
   if (!trans) {
     nbdkit_error ("reply with unexpected cookie %#" PRIx64,
rep.handle);
     return nbd_mark_dead (h);
-- 
2.14.3

On Mon, Dec 04, 2017 at 03:34:01PM -0600, Eric Blake
wrote:
> Now that we properly clean up 'trans' in the reader thread, we
> must not dereference 'trans' from the write thread at any point
> after trans has been added to the list unless we have grabbed
> it back off the list ourselves, or we risk an occasional
> use-after-free or even double free that valgrind can detect.
> 
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Fixes: cb189648f11df6c4f7287dcaed4bc856650e2c3b
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Perhaps I should also add more comments to the code about transfer of
> ownership of 'trans' between threads?
> 
>  plugins/nbd/nbd.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
> index e79042c..9d40e87 100644
> --- a/plugins/nbd/nbd.c
> +++ b/plugins/nbd/nbd.c
> @@ -227,6 +227,26 @@ nbd_mark_dead (struct handle *h)
>    return -1;
>  }
> 
> +/* Find and remove the transaction corresponding to cookie from the list.
*/
> +static struct transaction *
> +find_trans_by_cookie (struct handle *h, uint64_t cookie)
> +{
> +  struct transaction **ptr;
> +  struct transaction *trans;
> +
> +  nbd_lock (h);
> +  ptr = &h->trans;
> +  while ((trans = *ptr) != NULL) {
> +    if (cookie == trans->u.cookie)
> +      break;
> +    ptr = &trans->next;
> +  }
> +  if (trans)
> +    *ptr = trans->next;
> +  nbd_unlock (h);
> +  return trans;
> +}
> +
>  /* Send a request, return 0 on success or -1 on write failure. */
>  static int
>  nbd_request_raw (struct handle *h, uint32_t type, uint64_t offset,
> @@ -260,6 +280,8 @@ nbd_request_full (struct handle *h, uint32_t type,
uint64_t offset,
>  {
>    int err;
>    struct transaction *trans;
> +  int fd;
> +  uint64_t cookie;
> 
>    trans = calloc (1, sizeof *trans);
>    if (!trans) {
> @@ -282,9 +304,14 @@ nbd_request_full (struct handle *h, uint32_t type,
uint64_t offset,
>    }
>    trans->next = h->trans;
>    h->trans = trans;
> +  fd = trans->u.fds[0];
> +  cookie = trans->u.cookie;
>    nbd_unlock (h);
> -  if (nbd_request_raw (h, type, offset, count, trans->u.cookie,
req_buf) == 0)
> -    return trans->u.fds[0];
> +  if (nbd_request_raw (h, type, offset, count, cookie, req_buf) == 0)
> +    return fd;
> +  trans = find_trans_by_cookie (h, cookie);
> +  if (!trans)
> +    return nbd_mark_dead (h);
> 
>   err:
>    err = errno;
> @@ -309,7 +336,6 @@ static int
>  nbd_reply_raw (struct handle *h, int *fd)
>  {
>    struct reply rep;
> -  struct transaction **ptr;
>    struct transaction *trans;
>    void *buf;
>    uint32_t count;
> @@ -320,16 +346,7 @@ nbd_reply_raw (struct handle *h, int *fd)
>    if (be32toh (rep.magic) != NBD_REPLY_MAGIC)
>      return nbd_mark_dead (h);
>    nbdkit_debug ("received reply for cookie %#" PRIx64,
rep.handle);
> -  nbd_lock (h);
> -  ptr = &h->trans;
> -  while ((trans = *ptr) != NULL) {
> -    if (rep.handle == trans->u.cookie)
> -      break;
> -    ptr = &trans->next;
> -  }
> -  if (trans)
> -    *ptr = trans->next;
> -  nbd_unlock (h);
> +  trans = find_trans_by_cookie (h, rep.handle);
>    if (!trans) {
>      nbdkit_error ("reply with unexpected cookie %#" PRIx64,
rep.handle);
>      return nbd_mark_dead (h);
> -- 
> 2.14.3
I wasn't able to conclusively say that this fixes the problem
because it was quite rare and now isn't reproducing for me
at all.

Nevertheless, ACK.  Please push, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/