qemu-nbd: NULL nbd export pointer dereference after kill (TERMINATE)
PATCH 1/1 qemu-nbd: NULL nbd export pointer dereference after kill (TERMINATE)
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);--- blockdev-nbd.c | 5 +++-- include/block/nbd.h | 2 +- nbd.c | 5 ++++- qemu-nbd.c | 6 +++--- 4 files changed, 11 insertions(+), 7 deletions(-)
blockdev-nbd.c +3 -2
@@ -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); }
include/block/nbd.h +1 -1
@@ -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);
nbd.c +4 -1
@@ -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)
qemu-nbd.c +3 -3
@@ -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 );
Signature · 1 line
1.9.4.msysgit.0
Quoted 12 lines · Zhang Haoyu
After receive TERMINATE signal, qemu nbd state is set to TERMINATE, then in the main loop,
nbd_export_close->nbd_export_putis performed, but sometimesexp->refcountstill greater than zero afternbd_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
Il 09/08/2014 05:33, Zhang Haoyu ha scritto:
Quoted 12 lines
After receive TERMINATE signal, qemu nbd state is set to TERMINATE, then in the main loop,
nbd_export_close->nbd_export_putis performed, but sometimesexp->refcountstill greater than zero afternbd_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
Quoted 79 lines
Signed-off-by Zhang Haoyu <[email protected]>--- blockdev-nbd.c | 5 +++-- include/block/nbd.h | 2 +- nbd.c | 5 ++++- qemu-nbd.c | 6 +++--- 4 files changed, 11 insertions(+), 7 deletions(-)blockdev-nbd.c +3
-2@@ -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); }include/block/nbd.h +1
-1@@ -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);nbd.c +4
-1@@ -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)qemu-nbd.c +3
-3@@ -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 );