[Libguestfs] [nbdkit PATCH 1/4] server: Fix regression for NBD_OPT_INFO before NBD_OPT_GO
Eric Blake
eblake at redhat.com
Thu Sep 19 03:01:06 UTC 2019
Most known NBD clients do not bother with NBD_OPT_INFO (except for
clients like 'qemu-nbd --list' that don't ever intend to connect), but
go straight to NBD_OPT_GO. However, it's not too hard to hack up qemu
to add in an extra client step (whether info on the same name, or more
interestingly, info on a different name), as a patch against qemu
commit 6f214b30445:
| diff --git i/nbd/client.c w/nbd/client.c
| index f6733962b49b..425292ac5ea9 100644
| --- i/nbd/client.c
| +++ w/nbd/client.c
| @@ -1038,6 +1038,14 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
| * TLS). If it is not available, fall back to
| * NBD_OPT_LIST for nicer error messages about a missing
| * export, then use NBD_OPT_EXPORT_NAME. */
| + if (getenv ("HACK"))
| + info->name[0]++;
| + result = nbd_opt_info_or_go(ioc, NBD_OPT_INFO, info, errp);
| + if (getenv ("HACK"))
| + info->name[0]--;
| + if (result < 0) {
| + return -EINVAL;
| + }
| result = nbd_opt_info_or_go(ioc, NBD_OPT_GO, info, errp);
| if (result < 0) {
| return -EINVAL;
This works just fine in 1.14.0, where we call .open only once (so the
INFO and GO repeat calls into the same plugin handle), but in 1.14.1
it regressed into causing an assertion failure: we are now calling
.open a second time on a connection that is already opened.
$ nbdkit -rfv null &
$ hacked-qemu-io -f raw -r nbd://localhost -c quit
...
nbdkit: null[1]: debug: null: open readonly=1
nbdkit: backend.c:179: backend_open: Assertion `h->handle == NULL' failed.
Worse, on the mainline development, we have recently made it possible
for plugins to actively report different information for different
export names; for example, a plugin may choose to report different
answers for .can_write on export A than for export B; but if we share
cached handles, then an NBD_OPT_INFO on one export prevents correct
answers for NBD_OPT_GO on the second export name. (The HACK envvar in
my qemu modifications can be used to demonstrate cross-name requests,
which are even less likely in a real client).
The solution is to call .close after NBD_OPT_INFO, coupled with enough
glue logic to reset cached connection handles back to the state
expected by .open. This in turn means factoring out another backend_*
function, but also gives us an opportunity to change
backend_set_handle to no longer accept NULL.
The assertion failure is, to some extent, a possible denial of service
attack (one client can force nbdkit to exit by merely sending OPT_INFO
before OPT_GO, preventing the next client from connecting), although
this is mitigated by using TLS to weed out untrusted clients. Still,
the fact that we introduced a potential DoS attack while trying to fix
a traffic amplification security bug is not very nice.
Sadly, as there are no known clients that easily trigger this mode of
operation (OPT_INFO before OPT_GO), there is no easy way to cover this
via a testsuite addition. I may end up hacking something into libnbd.
Fixes: c05686f957
Signed-off-by: Eric Blake <eblake at redhat.com>
---
server/internal.h | 4 +++-
server/backend.c | 13 +++++++++++++
server/connections.c | 2 +-
server/filters.c | 5 +----
server/plugins.c | 4 ----
server/protocol-handshake-newstyle.c | 6 ++++++
6 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/server/internal.h b/server/internal.h
index c31bb340..da4fae19 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -334,9 +334,11 @@ extern int backend_open (struct backend *b, struct connection *conn,
__attribute__((__nonnull__ (1, 2)));
extern int backend_prepare (struct backend *b, struct connection *conn)
__attribute__((__nonnull__ (1, 2)));
+extern void backend_close (struct backend *b, struct connection *conn)
+ __attribute__((__nonnull__ (1, 2)));
extern void backend_set_handle (struct backend *b, struct connection *conn,
void *handle)
- __attribute__((__nonnull__ (1, 2 /* not 3 */)));
+ __attribute__((__nonnull__ (1, 2, 3)));
extern bool backend_valid_range (struct backend *b, struct connection *conn,
uint64_t offset, uint32_t count)
__attribute__((__nonnull__ (1, 2)));
diff --git a/server/backend.c b/server/backend.c
index 3b213bfb..64dbf7db 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -201,10 +201,23 @@ backend_prepare (struct backend *b, struct connection *conn)
return b->prepare (b, conn, h->can_write == 0);
}
+void
+backend_close (struct backend *b, struct connection *conn)
+{
+ struct b_conn_handle *h = &conn->handles[b->i];
+
+ debug ("%s: close", b->name);
+
+ b->close (b, conn);
+ memset (h, -1, sizeof *h);
+ h->handle = NULL;
+}
+
void
backend_set_handle (struct backend *b, struct connection *conn, void *handle)
{
assert (b->i < conn->nr_handles);
+ assert (conn->handles[b->i].handle == NULL);
conn->handles[b->i].handle = handle;
}
diff --git a/server/connections.c b/server/connections.c
index 819f7b86..3c4296ea 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -369,7 +369,7 @@ free_connection (struct connection *conn)
*/
if (!quit && connection_get_handle (conn, 0)) {
lock_request (conn);
- backend->close (backend, conn);
+ backend_close (backend, conn);
unlock_request (conn);
}
diff --git a/server/filters.c b/server/filters.c
index 1ee62829..1091c2dd 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -225,12 +225,9 @@ filter_close (struct backend *b, struct connection *conn)
struct backend_filter *f = container_of (b, struct backend_filter, backend);
void *handle = connection_get_handle (conn, b->i);
- debug ("%s: close", b->name);
-
if (handle && f->filter.close)
f->filter.close (handle);
- backend_set_handle (b, conn, NULL);
- b->next->close (b->next, conn);
+ backend_close (b->next, conn);
}
/* The next_functions structure contains pointers to backend
diff --git a/server/plugins.c b/server/plugins.c
index 9b44c548..727fb0e0 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -273,12 +273,8 @@ plugin_close (struct backend *b, struct connection *conn)
assert (connection_get_handle (conn, 0));
- debug ("close");
-
if (p->plugin.close)
p->plugin.close (connection_get_handle (conn, 0));
-
- backend_set_handle (b, conn, NULL);
}
static int64_t
diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c
index 87e0bcd7..06fc53ad 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -469,6 +469,12 @@ negotiate_handshake_newstyle_options (struct connection *conn)
if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1)
return -1;
+ if (option == NBD_OPT_INFO) {
+ if (backend->finalize (backend, conn) == -1)
+ return -1;
+ backend_close (backend, conn);
+ }
+
break;
case NBD_OPT_STRUCTURED_REPLY:
--
2.21.0
More information about the Libguestfs
mailing list