[Qemu-devel] [PATCH] qemu-nbd: NULL nbd export pointer dereference after kill (TERMINATE)

[Qemu-devel] [PATCH] qemu-nbd: NULL nbd export pointer dereference after kill (TERMINATE)

[Zhang Haoyu]

After receive TERMINATE signal, qemu nbd state is set to TERMINATE, then in the main loop, 
nbd_export_close -> nbd_export_put is performed, but sometimes exp->refcount still greater than zero after nbd_export_put,
so the qemu nbd state has not been set to TERMINATED, then in next cycle,  NULL exp will be dereference.
    do {
        main_loop_wait(false);
        if (state == TERMINATE) {
            state = TERMINATING;
            nbd_export_close(exp);
            nbd_export_put(exp);
            exp = NULL;
        }
    } while (state != TERMINATED);

Signed-off-by: Zhang Haoyu <zhanghy@sangfor.com>
---
 blockdev-nbd.c      | 5 +++--
 include/block/nbd.h | 2 +-
 nbd.c               | 5 ++++-
 qemu-nbd.c          | 6 +++---
 4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index b3a2474..c339081 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -65,8 +65,9 @@ static void nbd_close_notifier(Notifier *n, void *data)
     notifier_remove(&cn->n);
     QTAILQ_REMOVE(&close_notifiers, cn, next);
 
-    nbd_export_close(cn->exp);
-    nbd_export_put(cn->exp);
+    do {
+        cn->exp = nbd_export_put(cn->exp);
+    } while (cn->exp);
     g_free(cn);
 }
 
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 9e835d2..1912ef0 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -90,7 +90,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
                           void (*close)(NBDExport *));
 void nbd_export_close(NBDExport *exp);
 void nbd_export_get(NBDExport *exp);
-void nbd_export_put(NBDExport *exp);
+NBDExport *nbd_export_put(NBDExport *exp);
 
 BlockDriverState *nbd_export_get_blockdev(NBDExport *exp);
 
diff --git a/nbd.c b/nbd.c
index e7d1cee..2fccba5 100644
--- a/nbd.c
+++ b/nbd.c
@@ -991,7 +991,7 @@ void nbd_export_get(NBDExport *exp)
     exp->refcount++;
 }
 
-void nbd_export_put(NBDExport *exp)
+NBDExport *nbd_export_put(NBDExport *exp)
 {
     assert(exp->refcount > 0);
     if (exp->refcount == 1) {
@@ -1006,7 +1006,10 @@ void nbd_export_put(NBDExport *exp)
         }
 
         g_free(exp);
+        return NULL;
     }
+
+    return exp;
 }
 
 BlockDriverState *nbd_export_get_blockdev(NBDExport *exp)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 626e584..e1f3577 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -735,9 +735,9 @@ int main(int argc, char **argv)
         main_loop_wait(false);
         if (state == TERMINATE) {
             state = TERMINATING;
-            nbd_export_close(exp);
-            nbd_export_put(exp);
-            exp = NULL;
+            do {
+                exp = nbd_export_put(exp);
+            } while (exp);
         }
     } while (state != TERMINATED);
 
-- 
1.9.4.msysgit.0

Re: [Qemu-devel] [PATCH] qemu-nbd: NULL nbd export pointer dereference after kill (TERMINATE)

[Stefan Hajnoczi]

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

On Sat, Aug 09, 2014 at 11:33:14AM +0800, Zhang Haoyu wrote:
> After receive TERMINATE signal, qemu nbd state is set to TERMINATE, then in the main loop, 
> nbd_export_close -> nbd_export_put is performed, but sometimes exp->refcount still greater than zero after nbd_export_put,
> so the qemu nbd state has not been set to TERMINATED, then in next cycle,  NULL exp will be dereference.
>     do {
>         main_loop_wait(false);
>         if (state == TERMINATE) {
>             state = TERMINATING;
>             nbd_export_close(exp);
>             nbd_export_put(exp);
>             exp = NULL;
>         }
>     } while (state != TERMINATED);

Please describe the scenario where refcount is greater than zero.  The
commit description should describe the bug and how to reproduce it.

Do you have a test case that reproduces this crash?

Please look at QEMU's ./MAINTAINERS file and CC the appropriate people.
I have CCed Paolo Bonzini since he is the NBD maintainer.  If you do not
CC the maintainers they may miss your patch.

Please

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] qemu-nbd: NULL nbd export pointer dereference after kill (TERMINATE)

[Paolo Bonzini]

Il 09/08/2014 05:33, Zhang Haoyu ha scritto:
> After receive TERMINATE signal, qemu nbd state is set to TERMINATE, then in the main loop, 
> nbd_export_close -> nbd_export_put is performed, but sometimes exp->refcount still greater than zero after nbd_export_put,
> so the qemu nbd state has not been set to TERMINATED, then in next cycle,  NULL exp will be dereference.
>     do {
>         main_loop_wait(false);
>         if (state == TERMINATE) {
>             state = TERMINATING;
>             nbd_export_close(exp);
>             nbd_export_put(exp);
>             exp = NULL;
>         }
>     } while (state != TERMINATED);

This patch will cause use-after-frees if the reference count is dropped
by the legitimate owner.

Where is the NULL exp dereferenced?  Perhaps that's where a guard has to
be added.

Also, blockdev-nbd.c is not part of qemu-nbd.

Paolo

> Signed-off-by: Zhang Haoyu <zhanghy@sangfor.com>
> ---
>  blockdev-nbd.c      | 5 +++--
>  include/block/nbd.h | 2 +-
>  nbd.c               | 5 ++++-
>  qemu-nbd.c          | 6 +++---
>  4 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index b3a2474..c339081 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -65,8 +65,9 @@ static void nbd_close_notifier(Notifier *n, void *data)
>      notifier_remove(&cn->n);
>      QTAILQ_REMOVE(&close_notifiers, cn, next);
>  
> -    nbd_export_close(cn->exp);
> -    nbd_export_put(cn->exp);
> +    do {
> +        cn->exp = nbd_export_put(cn->exp);
> +    } while (cn->exp);
>      g_free(cn);
>  }
>  
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 9e835d2..1912ef0 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -90,7 +90,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
>                            void (*close)(NBDExport *));
>  void nbd_export_close(NBDExport *exp);
>  void nbd_export_get(NBDExport *exp);
> -void nbd_export_put(NBDExport *exp);
> +NBDExport *nbd_export_put(NBDExport *exp);
>  
>  BlockDriverState *nbd_export_get_blockdev(NBDExport *exp);
>  
> diff --git a/nbd.c b/nbd.c
> index e7d1cee..2fccba5 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -991,7 +991,7 @@ void nbd_export_get(NBDExport *exp)
>      exp->refcount++;
>  }
>  
> -void nbd_export_put(NBDExport *exp)
> +NBDExport *nbd_export_put(NBDExport *exp)
>  {
>      assert(exp->refcount > 0);
>      if (exp->refcount == 1) {
> @@ -1006,7 +1006,10 @@ void nbd_export_put(NBDExport *exp)
>          }
>  
>          g_free(exp);
> +        return NULL;
>      }
> +
> +    return exp;
>  }
>  
>  BlockDriverState *nbd_export_get_blockdev(NBDExport *exp)
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 626e584..e1f3577 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -735,9 +735,9 @@ int main(int argc, char **argv)
>          main_loop_wait(false);
>          if (state == TERMINATE) {
>              state = TERMINATING;
> -            nbd_export_close(exp);
> -            nbd_export_put(exp);
> -            exp = NULL;
> +            do {
> +                exp = nbd_export_put(exp);
> +            } while (exp);
>          }
>      } while (state != TERMINATED);
>  
>