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 <address@hidden>
> ---
>  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);
>  
>