nbd: Fully initialize client in case of failed negotiation
PATCH 1/1 nbd: Fully initialize client in case of failed negotiation
If a non-NBD client connects to qemu-nbd, we would end up with a SIGSEGV in nbd_cilent_put() because we were trying to unregister the client's association to the export, even though we skipped inserting the client into that list. Easy trigger in two terminals:
$ qemu-nbd -p 30001 --format=raw file $ nmap 127.0.0.1 -p 30001
nmap claims that it thinks it connected to a pago-services1 server (which probably means nmap could be updated to learn the NBD protocol and give a more accurate diagnosis of the open port - but that's not our problem), then terminates immediately, so our call to nbd_negotiate() fails. The fix is to reorder nbd_co_client_start() to ensure that all initialization occurs before we ever try talking to a client in nbd_negotiate(), so that the teardown sequence on negotiation failure doesn't fault while dereferencing a half-initialized object.
While debugging this, I also noticed that nbd_update_server_watch() called by nbd_client_closed() was still adding a channel to accept the next client, even when the state was no longer RUNNING. That is fixed by making nbd_can_accept() pay attention to the current state.
---
I'm planning to run a bisect to see which patch actually introduced the problem, but wanted to post the patch first to get review started.
nbd/server.c | 14 ++++++-------- qemu-nbd.c | 2 +- 2 files changed, 7 insertions(+), 9 deletions(-)
nbd/server.c +6 -8
@@ -1601,16 +1601,14 @@ static coroutine_fn void nbd_co_client_start(void *opaque)if (exp) {nbd_export_get (exp); - } -if (nbd_negotiate (data)) { -client_close (client); - goto out; - } -qemu_co_mutex_init (&client->send_lock); - -if (exp) {QTAILQ_INSERT_TAIL (&exp->clients, client, next); } +qemu_co_mutex_init (&client->send_lock); + +if (nbd_negotiate (data)) { +client_close (client); + goto out; + }nbd_client_receive_next_request (client);
qemu-nbd.c +1 -1
@@ -322,7 +322,7 @@ out:static int nbd_can_accept (void ) { -return nb_fds < shared; +return state ==RUNNING && nb_fds < shared; }static void nbd_export_closed (NBDExport *exp)
Signature · 1 line
2.9.4
Quoted 10 lines · Eric Blake
If a non-NBD client connects to qemu-nbd, we would end up with a SIGSEGV in
nbd_cilent_put()because we were trying to unregister the client's association to the export, even though we skipped inserting the client into that list. Easy trigger in two terminals:$ qemu-nbd -p 30001 --format=raw file $ nmap 127.0.0.1 -p 30001
Quoted 7 lines
Signed-off-by Eric Blake <[email protected]>---
I'm planning to run a bisect to see which patch actually introduced the problem, but wanted to post the patch first to get review started.
Looks like the problem of split initialization has existed since at least 2.6; commit 1a6245a split up the QTAILQ_INSERT_TAIL and nbd_export_get as part of refactoring to create nbd_co_client_start. But even trying the commit before that, I still got a different assertion failure:
qemu-nbd: nbd/server.c:521: nbd_client_put: Assertion `client->closing' failed.
Going all the way back to 2.5 worked, but 2.6 was far enough back that I didn't want to bisect further which commit first broke probes from nmap.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 8 lines · Eric Blake
If a non-NBD client connects to qemu-nbd, we would end up with a SIGSEGV in
nbd_cilent_put()because we were trying to unregister the client's association to the export, even though we skipped inserting the client into that list. Easy trigger in two terminals:$ qemu-nbd -p 30001 --format=raw file $ nmap 127.0.0.1 -p 30001
Since this is now part of a CVE fix, I'm adding qemu-stable in cc.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org