[PATCH for-9.1 v3 0/2] NBD CVE-2024-7409

[PATCH for-9.1 v3 0/2] NBD CVE-2024-7409

[Eric Blake]

v2 was here:
https://lists.gnu.org/archive/html/qemu-devel/2024-08/msg00253.html

Since then:
 - CVE number assigned
 - drop old patch 1. Instead of tracking nbd_server generation, the
   code now ensures that nbd_server can't be set to NULL until all
   clients have disconnected
 - rewrite to force qio shutdown coupled with AIO_WAIT to ensure all
   clients actually disconnect quickly (from the server's
   perspective. A client may still hold its socket open longer, but
   will eventually see EPIPE or EOF when finally using it)
 - patch 2 is optional, although I like the notion of a doubly-linked
   list (where the client has to remember an opaque pointer) over a
   singly-linked one (where the client is unchanged, but a lot of
   repeated client connect/disconnect over a long-lived server can
   chew up memory and slow down the eventual nbd-server-stop)

Eric Blake (2):
  nbd: CVE-2024-7409: Close stray client sockets at server shutdown
  nbd: Clean up clients more efficiently

 include/block/nbd.h |  4 +++-
 blockdev-nbd.c      | 39 +++++++++++++++++++++++++++++++++++++--
 nbd/server.c        | 15 ++++++++++++---
 qemu-nbd.c          |  2 +-
 4 files changed, 53 insertions(+), 7 deletions(-)

-- 
2.45.2

[PATCH v3 1/2] nbd: CVE-2024-7409: Close stray client sockets at server shutdown

[Eric Blake]

A malicious client can attempt to connect to an NBD server, and then
intentionally delay progress in the handshake, including if it does
not know the TLS secrets.  Although this behavior can be bounded by
the max-connections parameter, the QMP nbd-server-start currently
defaults to unlimited incoming client connections.  Worse, if the
client waits to close the socket until after the QMP nbd-server-stop
command is executed, qemu will then SEGV when trying to dereference
the NULL nbd_server global whish is no longer present, which amounts
to a denial of service attack.  If another NBD server is started
before the malicious client disconnects, I cannot rule out additional
adverse effects when the old client interferes with the connection
count of the new server.

For environments without this patch, the CVE can be mitigated by
ensuring (such as via a firewall) that only trusted clients can
connect to an NBD server.  Note that using frameworks like libvirt
that ensure that TLS is used and that nbd-server-stop is not executed
while any trusted clients are still connected will only help if there
is also no possibility for an untrusted client to open a connection
but then stall on the NBD handshake.

This patch fixes the problem by tracking all client sockets opened
while the server is running, and forcefully closing any such sockets
remaining without a completed handshake at the time of
nbd-server-stop, then waiting until the coroutines servicing those
sockets notice the state change.  nbd-server-stop may now take
slightly longer to execute, but the extra time is independent of
client response behaviors, and is generally no worse than the time
already taken by the blk_exp_close_all_type() that disconnects all
clients that completed handshakes (since that code also has an
AIO_WAIT_WHILE_UNLOCKED).  For a long-running server with lots of
clients rapidly connecting and disconnecting, the memory used to track
all client sockets can result in some memory overhead, but it is not a
leak; the next patch will further optimize that by cleaning up memory
as clients go away.  At any rate, this patch in isolation is
sufficient to fix the CVE.

This patch relies heavily on the fact that nbd/server.c guarantees
that it only calls nbd_blockdev_client_closed() from the main loop
(see the assertion in nbd_client_put() and the hoops used in
nbd_client_put_nonzero() to achieve that); if we did not have that
guarantee, we would also need a mutex protecting our accesses of the
list of connections to survive re-entrancy from independent iothreads.

Although I did not actually try to test old builds, it looks like this
problem has existed since at least commit 862172f45c (v2.12.0, 2017) -
even back in that patch to start using a QIONetListener to handle
listening on multiple sockets, nbd_server_free() was already unaware
that the nbd_blockdev_client_closed callback can be reached later by a
client thread that has not completed handshakes (and therefore the
client's socket never got added to the list closed in
nbd_export_close_all), despite that patch intentionally tearing down
the QIONetListener to prevent new clients.

Reported-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Fixes: CVE-2024-7409
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 blockdev-nbd.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 213012435f4..b8f00f402c6 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -21,12 +21,18 @@
 #include "io/channel-socket.h"
 #include "io/net-listener.h"

+typedef struct NBDConn {
+    QIOChannelSocket *cioc;
+    QSLIST_ENTRY(NBDConn) next;
+} NBDConn;
+
 typedef struct NBDServerData {
     QIONetListener *listener;
     QCryptoTLSCreds *tlscreds;
     char *tlsauthz;
     uint32_t max_connections;
     uint32_t connections;
+    QSLIST_HEAD(, NBDConn) conns;
 } NBDServerData;

 static NBDServerData *nbd_server;
@@ -51,6 +57,8 @@ int nbd_server_max_connections(void)

 static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
 {
+    assert(qemu_in_main_thread() && nbd_server);
+
     nbd_client_put(client);
     assert(nbd_server->connections > 0);
     nbd_server->connections--;
@@ -60,7 +68,13 @@ static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
 static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
                        gpointer opaque)
 {
+    NBDConn *conn = g_new0(NBDConn, 1);
+
+    assert(qemu_in_main_thread() && nbd_server);
     nbd_server->connections++;
+    object_ref(OBJECT(cioc));
+    conn->cioc = cioc;
+    QSLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
     nbd_update_server_watch(nbd_server);

     qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
@@ -83,8 +97,24 @@ static void nbd_server_free(NBDServerData *server)
         return;
     }

+    /*
+     * Forcefully close the listener socket, and any clients that have
+     * not yet disconnected on their own.
+     */
     qio_net_listener_disconnect(server->listener);
     object_unref(OBJECT(server->listener));
+    while (!QSLIST_EMPTY(&server->conns)) {
+        NBDConn *conn = QSLIST_FIRST(&server->conns);
+
+        qio_channel_shutdown(QIO_CHANNEL(conn->cioc), QIO_CHANNEL_SHUTDOWN_BOTH,
+                             NULL);
+        object_unref(OBJECT(conn->cioc));
+        QSLIST_REMOVE_HEAD(&server->conns, next);
+        g_free(conn);
+    }
+
+    AIO_WAIT_WHILE_UNLOCKED(NULL, server->connections > 0);
+
     if (server->tlscreds) {
         object_unref(OBJECT(server->tlscreds));
     }
-- 
2.45.2

Re: [PATCH v3 1/2] nbd: CVE-2024-7409: Close stray client sockets at server shutdown

[Daniel P. Berrangé]

On Mon, Aug 05, 2024 at 09:21:35PM -0500, Eric Blake wrote:
> A malicious client can attempt to connect to an NBD server, and then
> intentionally delay progress in the handshake, including if it does
> not know the TLS secrets.  Although this behavior can be bounded by
> the max-connections parameter, the QMP nbd-server-start currently
> defaults to unlimited incoming client connections.  Worse, if the
> client waits to close the socket until after the QMP nbd-server-stop
> command is executed, qemu will then SEGV when trying to dereference
> the NULL nbd_server global whish is no longer present, which amounts
> to a denial of service attack.  If another NBD server is started
> before the malicious client disconnects, I cannot rule out additional
> adverse effects when the old client interferes with the connection
> count of the new server.
> 
> For environments without this patch, the CVE can be mitigated by
> ensuring (such as via a firewall) that only trusted clients can
> connect to an NBD server.  Note that using frameworks like libvirt
> that ensure that TLS is used and that nbd-server-stop is not executed
> while any trusted clients are still connected will only help if there
> is also no possibility for an untrusted client to open a connection
> but then stall on the NBD handshake.
> 
> This patch fixes the problem by tracking all client sockets opened
> while the server is running, and forcefully closing any such sockets
> remaining without a completed handshake at the time of
> nbd-server-stop, then waiting until the coroutines servicing those
> sockets notice the state change.  nbd-server-stop may now take
> slightly longer to execute, but the extra time is independent of
> client response behaviors, and is generally no worse than the time
> already taken by the blk_exp_close_all_type() that disconnects all
> clients that completed handshakes (since that code also has an
> AIO_WAIT_WHILE_UNLOCKED).  For a long-running server with lots of
> clients rapidly connecting and disconnecting, the memory used to track
> all client sockets can result in some memory overhead, but it is not a
> leak; the next patch will further optimize that by cleaning up memory
> as clients go away.  At any rate, this patch in isolation is
> sufficient to fix the CVE.
> 
> This patch relies heavily on the fact that nbd/server.c guarantees
> that it only calls nbd_blockdev_client_closed() from the main loop
> (see the assertion in nbd_client_put() and the hoops used in
> nbd_client_put_nonzero() to achieve that); if we did not have that
> guarantee, we would also need a mutex protecting our accesses of the
> list of connections to survive re-entrancy from independent iothreads.
> 
> Although I did not actually try to test old builds, it looks like this
> problem has existed since at least commit 862172f45c (v2.12.0, 2017) -
> even back in that patch to start using a QIONetListener to handle
> listening on multiple sockets, nbd_server_free() was already unaware
> that the nbd_blockdev_client_closed callback can be reached later by a
> client thread that has not completed handshakes (and therefore the
> client's socket never got added to the list closed in
> nbd_export_close_all), despite that patch intentionally tearing down
> the QIONetListener to prevent new clients.
> 
> Reported-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> Fixes: CVE-2024-7409
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  blockdev-nbd.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v3 2/2] nbd: Clean up clients more efficiently

[Eric Blake]

Since an NBD server may be long-living, serving clients that
repeatedly connect and disconnect, it can be more efficient to clean
up after each client disconnects, rather than storing a list of
resources to clean up when the server exits.  Rewrite the list of
known clients to be double-linked so that we can get O(1) deletion to
keep the list pruned to size as clients exit.  This in turn requires
each client to track an opaque pointer of owner information (although
qemu-nbd doesn't need to refer to it).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  4 +++-
 blockdev-nbd.c      | 27 ++++++++++++++++-----------
 nbd/server.c        | 15 ++++++++++++---
 qemu-nbd.c          |  2 +-
 4 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4e7bd6342f9..7dce9b9c35b 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -405,7 +405,9 @@ NBDExport *nbd_export_find(const char *name);
 void nbd_client_new(QIOChannelSocket *sioc,
                     QCryptoTLSCreds *tlscreds,
                     const char *tlsauthz,
-                    void (*close_fn)(NBDClient *, bool));
+                    void (*close_fn)(NBDClient *, bool),
+                    void *owner);
+void *nbd_client_owner(NBDClient *client);
 void nbd_client_get(NBDClient *client);
 void nbd_client_put(NBDClient *client);

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index b8f00f402c6..660f89d881e 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -23,7 +23,7 @@

 typedef struct NBDConn {
     QIOChannelSocket *cioc;
-    QSLIST_ENTRY(NBDConn) next;
+    QLIST_ENTRY(NBDConn) next;
 } NBDConn;

 typedef struct NBDServerData {
@@ -32,10 +32,11 @@ typedef struct NBDServerData {
     char *tlsauthz;
     uint32_t max_connections;
     uint32_t connections;
-    QSLIST_HEAD(, NBDConn) conns;
+    QLIST_HEAD(, NBDConn) conns;
 } NBDServerData;

 static NBDServerData *nbd_server;
+static uint32_t nbd_cookie; /* Generation count of nbd_server */
 static int qemu_nbd_connections = -1; /* Non-negative if this is qemu-nbd */

 static void nbd_update_server_watch(NBDServerData *s);
@@ -57,10 +58,16 @@ int nbd_server_max_connections(void)

 static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
 {
+    NBDConn *conn = nbd_client_owner(client);
+
     assert(qemu_in_main_thread() && nbd_server);

+    object_unref(OBJECT(conn->cioc));
+    QLIST_REMOVE(conn, next);
+    g_free(conn);
+
     nbd_client_put(client);
-    assert(nbd_server->connections > 0);
+    assert(nbd_server && nbd_server->connections > 0);
     nbd_server->connections--;
     nbd_update_server_watch(nbd_server);
 }
@@ -74,12 +81,12 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
     nbd_server->connections++;
     object_ref(OBJECT(cioc));
     conn->cioc = cioc;
-    QSLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
+    QLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
     nbd_update_server_watch(nbd_server);

     qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
     nbd_client_new(cioc, nbd_server->tlscreds, nbd_server->tlsauthz,
-                   nbd_blockdev_client_closed);
+                   nbd_blockdev_client_closed, conn);
 }

 static void nbd_update_server_watch(NBDServerData *s)
@@ -93,6 +100,8 @@ static void nbd_update_server_watch(NBDServerData *s)

 static void nbd_server_free(NBDServerData *server)
 {
+    NBDConn *conn, *tmp;
+
     if (!server) {
         return;
     }
@@ -103,14 +112,9 @@ static void nbd_server_free(NBDServerData *server)
      */
     qio_net_listener_disconnect(server->listener);
     object_unref(OBJECT(server->listener));
-    while (!QSLIST_EMPTY(&server->conns)) {
-        NBDConn *conn = QSLIST_FIRST(&server->conns);
-
+    QLIST_FOREACH_SAFE(conn, &server->conns, next, tmp) {
         qio_channel_shutdown(QIO_CHANNEL(conn->cioc), QIO_CHANNEL_SHUTDOWN_BOTH,
                              NULL);
-        object_unref(OBJECT(conn->cioc));
-        QSLIST_REMOVE_HEAD(&server->conns, next);
-        g_free(conn);
     }

     AIO_WAIT_WHILE_UNLOCKED(NULL, server->connections > 0);
@@ -119,6 +123,7 @@ static void nbd_server_free(NBDServerData *server)
         object_unref(OBJECT(server->tlscreds));
     }
     g_free(server->tlsauthz);
+    nbd_cookie++;

     g_free(server);
 }
diff --git a/nbd/server.c b/nbd/server.c
index 892797bb111..90f48b42a47 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -124,6 +124,7 @@ struct NBDMetaContexts {
 struct NBDClient {
     int refcount; /* atomic */
     void (*close_fn)(NBDClient *client, bool negotiated);
+    void *owner;

     QemuMutex lock;

@@ -3205,14 +3206,15 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
 }

 /*
- * Create a new client listener using the given channel @sioc.
+ * Create a new client listener using the given channel @sioc and @owner.
  * Begin servicing it in a coroutine.  When the connection closes, call
- * @close_fn with an indication of whether the client completed negotiation.
+ * @close_fn and an indication of whether the client completed negotiation.
  */
 void nbd_client_new(QIOChannelSocket *sioc,
                     QCryptoTLSCreds *tlscreds,
                     const char *tlsauthz,
-                    void (*close_fn)(NBDClient *, bool))
+                    void (*close_fn)(NBDClient *, bool),
+                    void *owner)
 {
     NBDClient *client;
     Coroutine *co;
@@ -3231,7 +3233,14 @@ void nbd_client_new(QIOChannelSocket *sioc,
     client->ioc = QIO_CHANNEL(sioc);
     object_ref(OBJECT(client->ioc));
     client->close_fn = close_fn;
+    client->owner = owner;

     co = qemu_coroutine_create(nbd_co_client_start, client);
     qemu_coroutine_enter(co);
 }
+
+void *
+nbd_client_owner(NBDClient *client)
+{
+    return client->owner;
+}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index d7b3ccab21c..da6e36a2a34 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -390,7 +390,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,

     nb_fds++;
     nbd_update_server_watch();
-    nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed);
+    nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed, NULL);
 }

 static void nbd_update_server_watch(void)
-- 
2.45.2

Re: [PATCH v3 2/2] nbd: Clean up clients more efficiently

[Eric Blake]

On Mon, Aug 05, 2024 at 09:21:36PM GMT, Eric Blake wrote:
> Since an NBD server may be long-living, serving clients that
> repeatedly connect and disconnect, it can be more efficient to clean
> up after each client disconnects, rather than storing a list of
> resources to clean up when the server exits.  Rewrite the list of
> known clients to be double-linked so that we can get O(1) deletion to
> keep the list pruned to size as clients exit.  This in turn requires
> each client to track an opaque pointer of owner information (although
> qemu-nbd doesn't need to refer to it).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/nbd.h |  4 +++-
>  blockdev-nbd.c      | 27 ++++++++++++++++-----------
>  nbd/server.c        | 15 ++++++++++++---
>  qemu-nbd.c          |  2 +-
>  4 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 4e7bd6342f9..7dce9b9c35b 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -405,7 +405,9 @@ NBDExport *nbd_export_find(const char *name);
>  void nbd_client_new(QIOChannelSocket *sioc,
>                      QCryptoTLSCreds *tlscreds,
>                      const char *tlsauthz,
> -                    void (*close_fn)(NBDClient *, bool));
> +                    void (*close_fn)(NBDClient *, bool),
> +                    void *owner);
> +void *nbd_client_owner(NBDClient *client);
>  void nbd_client_get(NBDClient *client);
>  void nbd_client_put(NBDClient *client);
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index b8f00f402c6..660f89d881e 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -23,7 +23,7 @@
> 
>  typedef struct NBDConn {
>      QIOChannelSocket *cioc;
> -    QSLIST_ENTRY(NBDConn) next;
> +    QLIST_ENTRY(NBDConn) next;
>  } NBDConn;
> 
>  typedef struct NBDServerData {
> @@ -32,10 +32,11 @@ typedef struct NBDServerData {
>      char *tlsauthz;
>      uint32_t max_connections;
>      uint32_t connections;
> -    QSLIST_HEAD(, NBDConn) conns;
> +    QLIST_HEAD(, NBDConn) conns;
>  } NBDServerData;
> 
>  static NBDServerData *nbd_server;
> +static uint32_t nbd_cookie; /* Generation count of nbd_server */

Dead line leftover from v2; gone in my tree now.

>  static int qemu_nbd_connections = -1; /* Non-negative if this is qemu-nbd */
> 
>  static void nbd_update_server_watch(NBDServerData *s);
> @@ -57,10 +58,16 @@ int nbd_server_max_connections(void)
> 
>  static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
>  {
> +    NBDConn *conn = nbd_client_owner(client);
> +
>      assert(qemu_in_main_thread() && nbd_server);
> 
> +    object_unref(OBJECT(conn->cioc));
> +    QLIST_REMOVE(conn, next);
> +    g_free(conn);
> +
>      nbd_client_put(client);
> -    assert(nbd_server->connections > 0);
> +    assert(nbd_server && nbd_server->connections > 0);

The 'nbd_server && ' in this hunk is redundant with a couple lines above.

>      nbd_server->connections--;
>      nbd_update_server_watch(nbd_server);
>  }
> @@ -74,12 +81,12 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
>      nbd_server->connections++;
>      object_ref(OBJECT(cioc));
>      conn->cioc = cioc;
> -    QSLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
> +    QLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
>      nbd_update_server_watch(nbd_server);
> 
>      qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
>      nbd_client_new(cioc, nbd_server->tlscreds, nbd_server->tlsauthz,
> -                   nbd_blockdev_client_closed);
> +                   nbd_blockdev_client_closed, conn);
>  }
> 
>  static void nbd_update_server_watch(NBDServerData *s)
> @@ -93,6 +100,8 @@ static void nbd_update_server_watch(NBDServerData *s)
> 
>  static void nbd_server_free(NBDServerData *server)
>  {
> +    NBDConn *conn, *tmp;
> +
>      if (!server) {
>          return;
>      }
> @@ -103,14 +112,9 @@ static void nbd_server_free(NBDServerData *server)
>       */
>      qio_net_listener_disconnect(server->listener);
>      object_unref(OBJECT(server->listener));
> -    while (!QSLIST_EMPTY(&server->conns)) {
> -        NBDConn *conn = QSLIST_FIRST(&server->conns);
> -
> +    QLIST_FOREACH_SAFE(conn, &server->conns, next, tmp) {
>          qio_channel_shutdown(QIO_CHANNEL(conn->cioc), QIO_CHANNEL_SHUTDOWN_BOTH,
>                               NULL);
> -        object_unref(OBJECT(conn->cioc));
> -        QSLIST_REMOVE_HEAD(&server->conns, next);
> -        g_free(conn);
>      }
> 
>      AIO_WAIT_WHILE_UNLOCKED(NULL, server->connections > 0);
> @@ -119,6 +123,7 @@ static void nbd_server_free(NBDServerData *server)
>          object_unref(OBJECT(server->tlscreds));
>      }
>      g_free(server->tlsauthz);
> +    nbd_cookie++;

One more dead line.

> 
>      g_free(server);
>  }
> diff --git a/nbd/server.c b/nbd/server.c
> index 892797bb111..90f48b42a47 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -124,6 +124,7 @@ struct NBDMetaContexts {
>  struct NBDClient {
>      int refcount; /* atomic */
>      void (*close_fn)(NBDClient *client, bool negotiated);
> +    void *owner;
> 
>      QemuMutex lock;
> 
> @@ -3205,14 +3206,15 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
>  }
> 
>  /*
> - * Create a new client listener using the given channel @sioc.
> + * Create a new client listener using the given channel @sioc and @owner.
>   * Begin servicing it in a coroutine.  When the connection closes, call
> - * @close_fn with an indication of whether the client completed negotiation.
> + * @close_fn and an indication of whether the client completed negotiation.
>   */
>  void nbd_client_new(QIOChannelSocket *sioc,
>                      QCryptoTLSCreds *tlscreds,
>                      const char *tlsauthz,
> -                    void (*close_fn)(NBDClient *, bool))
> +                    void (*close_fn)(NBDClient *, bool),
> +                    void *owner)
>  {
>      NBDClient *client;
>      Coroutine *co;
> @@ -3231,7 +3233,14 @@ void nbd_client_new(QIOChannelSocket *sioc,
>      client->ioc = QIO_CHANNEL(sioc);
>      object_ref(OBJECT(client->ioc));
>      client->close_fn = close_fn;
> +    client->owner = owner;
> 
>      co = qemu_coroutine_create(nbd_co_client_start, client);
>      qemu_coroutine_enter(co);
>  }
> +
> +void *
> +nbd_client_owner(NBDClient *client)
> +{
> +    return client->owner;
> +}
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index d7b3ccab21c..da6e36a2a34 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -390,7 +390,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
> 
>      nb_fds++;
>      nbd_update_server_watch();
> -    nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed);
> +    nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed, NULL);
>  }
> 
>  static void nbd_update_server_watch(void)
> -- 
> 2.45.2
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v3 2/2] nbd: Clean up clients more efficiently

[Daniel P. Berrangé]

On Mon, Aug 05, 2024 at 09:21:36PM -0500, Eric Blake wrote:
> Since an NBD server may be long-living, serving clients that
> repeatedly connect and disconnect, it can be more efficient to clean
> up after each client disconnects, rather than storing a list of
> resources to clean up when the server exits.  Rewrite the list of
> known clients to be double-linked so that we can get O(1) deletion to
> keep the list pruned to size as clients exit.  This in turn requires
> each client to track an opaque pointer of owner information (although
> qemu-nbd doesn't need to refer to it).

I tend to feel that this needs to be squashed into the previous
patch.  The previous patch effectively creates unbounded memory
usage in the NBD server. ie consider a client that connects and
immediately disconnects, not sending any data, in a tight loop.
It will "leak" NBDConn & QIOChanelSocket pointers for each
iteration of the loop, only to be cleaned up when the NBD Server
is shutdown.

Especially given that we tagged the previous commit with a CVE
and not this commit,  people could be misled into backporting
only the former commit leaving them open to the leak.

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/nbd.h |  4 +++-
>  blockdev-nbd.c      | 27 ++++++++++++++++-----------
>  nbd/server.c        | 15 ++++++++++++---
>  qemu-nbd.c          |  2 +-
>  4 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 4e7bd6342f9..7dce9b9c35b 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -405,7 +405,9 @@ NBDExport *nbd_export_find(const char *name);
>  void nbd_client_new(QIOChannelSocket *sioc,
>                      QCryptoTLSCreds *tlscreds,
>                      const char *tlsauthz,
> -                    void (*close_fn)(NBDClient *, bool));
> +                    void (*close_fn)(NBDClient *, bool),
> +                    void *owner);
> +void *nbd_client_owner(NBDClient *client);
>  void nbd_client_get(NBDClient *client);
>  void nbd_client_put(NBDClient *client);
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index b8f00f402c6..660f89d881e 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -23,7 +23,7 @@
> 
>  typedef struct NBDConn {
>      QIOChannelSocket *cioc;
> -    QSLIST_ENTRY(NBDConn) next;
> +    QLIST_ENTRY(NBDConn) next;
>  } NBDConn;
> 
>  typedef struct NBDServerData {
> @@ -32,10 +32,11 @@ typedef struct NBDServerData {
>      char *tlsauthz;
>      uint32_t max_connections;
>      uint32_t connections;
> -    QSLIST_HEAD(, NBDConn) conns;
> +    QLIST_HEAD(, NBDConn) conns;
>  } NBDServerData;
> 
>  static NBDServerData *nbd_server;
> +static uint32_t nbd_cookie; /* Generation count of nbd_server */
>  static int qemu_nbd_connections = -1; /* Non-negative if this is qemu-nbd */
> 
>  static void nbd_update_server_watch(NBDServerData *s);
> @@ -57,10 +58,16 @@ int nbd_server_max_connections(void)
> 
>  static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
>  {
> +    NBDConn *conn = nbd_client_owner(client);
> +
>      assert(qemu_in_main_thread() && nbd_server);
> 
> +    object_unref(OBJECT(conn->cioc));
> +    QLIST_REMOVE(conn, next);
> +    g_free(conn);
> +
>      nbd_client_put(client);
> -    assert(nbd_server->connections > 0);
> +    assert(nbd_server && nbd_server->connections > 0);
>      nbd_server->connections--;
>      nbd_update_server_watch(nbd_server);
>  }
> @@ -74,12 +81,12 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
>      nbd_server->connections++;
>      object_ref(OBJECT(cioc));
>      conn->cioc = cioc;
> -    QSLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
> +    QLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
>      nbd_update_server_watch(nbd_server);
> 
>      qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
>      nbd_client_new(cioc, nbd_server->tlscreds, nbd_server->tlsauthz,
> -                   nbd_blockdev_client_closed);
> +                   nbd_blockdev_client_closed, conn);
>  }
> 
>  static void nbd_update_server_watch(NBDServerData *s)
> @@ -93,6 +100,8 @@ static void nbd_update_server_watch(NBDServerData *s)
> 
>  static void nbd_server_free(NBDServerData *server)
>  {
> +    NBDConn *conn, *tmp;
> +
>      if (!server) {
>          return;
>      }
> @@ -103,14 +112,9 @@ static void nbd_server_free(NBDServerData *server)
>       */
>      qio_net_listener_disconnect(server->listener);
>      object_unref(OBJECT(server->listener));
> -    while (!QSLIST_EMPTY(&server->conns)) {
> -        NBDConn *conn = QSLIST_FIRST(&server->conns);
> -
> +    QLIST_FOREACH_SAFE(conn, &server->conns, next, tmp) {
>          qio_channel_shutdown(QIO_CHANNEL(conn->cioc), QIO_CHANNEL_SHUTDOWN_BOTH,
>                               NULL);
> -        object_unref(OBJECT(conn->cioc));
> -        QSLIST_REMOVE_HEAD(&server->conns, next);
> -        g_free(conn);
>      }
> 
>      AIO_WAIT_WHILE_UNLOCKED(NULL, server->connections > 0);
> @@ -119,6 +123,7 @@ static void nbd_server_free(NBDServerData *server)
>          object_unref(OBJECT(server->tlscreds));
>      }
>      g_free(server->tlsauthz);
> +    nbd_cookie++;
> 
>      g_free(server);
>  }
> diff --git a/nbd/server.c b/nbd/server.c
> index 892797bb111..90f48b42a47 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -124,6 +124,7 @@ struct NBDMetaContexts {
>  struct NBDClient {
>      int refcount; /* atomic */
>      void (*close_fn)(NBDClient *client, bool negotiated);
> +    void *owner;
> 
>      QemuMutex lock;
> 
> @@ -3205,14 +3206,15 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
>  }
> 
>  /*
> - * Create a new client listener using the given channel @sioc.
> + * Create a new client listener using the given channel @sioc and @owner.
>   * Begin servicing it in a coroutine.  When the connection closes, call
> - * @close_fn with an indication of whether the client completed negotiation.
> + * @close_fn and an indication of whether the client completed negotiation.
>   */
>  void nbd_client_new(QIOChannelSocket *sioc,
>                      QCryptoTLSCreds *tlscreds,
>                      const char *tlsauthz,
> -                    void (*close_fn)(NBDClient *, bool))
> +                    void (*close_fn)(NBDClient *, bool),
> +                    void *owner)
>  {
>      NBDClient *client;
>      Coroutine *co;
> @@ -3231,7 +3233,14 @@ void nbd_client_new(QIOChannelSocket *sioc,
>      client->ioc = QIO_CHANNEL(sioc);
>      object_ref(OBJECT(client->ioc));
>      client->close_fn = close_fn;
> +    client->owner = owner;
> 
>      co = qemu_coroutine_create(nbd_co_client_start, client);
>      qemu_coroutine_enter(co);
>  }
> +
> +void *
> +nbd_client_owner(NBDClient *client)
> +{
> +    return client->owner;
> +}
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index d7b3ccab21c..da6e36a2a34 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -390,7 +390,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
> 
>      nb_fds++;
>      nbd_update_server_watch();
> -    nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed);
> +    nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed, NULL);
>  }
> 
>  static void nbd_update_server_watch(void)
> -- 
> 2.45.2
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 2/2] nbd: Clean up clients more efficiently

[Eric Blake]

On Tue, Aug 06, 2024 at 10:32:54AM GMT, Daniel P. Berrangé wrote:
> On Mon, Aug 05, 2024 at 09:21:36PM -0500, Eric Blake wrote:
> > Since an NBD server may be long-living, serving clients that
> > repeatedly connect and disconnect, it can be more efficient to clean
> > up after each client disconnects, rather than storing a list of
> > resources to clean up when the server exits.  Rewrite the list of
> > known clients to be double-linked so that we can get O(1) deletion to
> > keep the list pruned to size as clients exit.  This in turn requires
> > each client to track an opaque pointer of owner information (although
> > qemu-nbd doesn't need to refer to it).
> 
> I tend to feel that this needs to be squashed into the previous
> patch.  The previous patch effectively creates unbounded memory
> usage in the NBD server. ie consider a client that connects and
> immediately disconnects, not sending any data, in a tight loop.
> It will "leak" NBDConn & QIOChanelSocket pointers for each
> iteration of the loop, only to be cleaned up when the NBD Server
> is shutdown.
> 
> Especially given that we tagged the previous commit with a CVE
> and not this commit,  people could be misled into backporting
> only the former commit leaving them open to the leak.

Makes sense.  Will respin v4 along those lines; although I plan to
refactor slightly: have patch 1 pick up the part of this patch that
allows server.c to track a client's owner, then patch 2 be the CVE fix
creating the doubly-linked list of QIOSocketChannel wrappers as
owners.  Also, I realized that nbd_server->connections == 0 is now
effectively redundant with QLIST_EMPTY(&nbd_server->conns), so that's
another cleanup to squash in.

> > @@ -103,14 +112,9 @@ static void nbd_server_free(NBDServerData *server)
> >       */
> >      qio_net_listener_disconnect(server->listener);
> >      object_unref(OBJECT(server->listener));
> > -    while (!QSLIST_EMPTY(&server->conns)) {
> > -        NBDConn *conn = QSLIST_FIRST(&server->conns);
> > -
> > +    QLIST_FOREACH_SAFE(conn, &server->conns, next, tmp) {
> >          qio_channel_shutdown(QIO_CHANNEL(conn->cioc), QIO_CHANNEL_SHUTDOWN_BOTH,
> >                               NULL);
> > -        object_unref(OBJECT(conn->cioc));
> > -        QSLIST_REMOVE_HEAD(&server->conns, next);
> > -        g_free(conn);
> >      }
> > 
> >      AIO_WAIT_WHILE_UNLOCKED(NULL, server->connections > 0);

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v3 2/2] nbd: Clean up clients more efficiently

[Eric Blake]

On Tue, Aug 06, 2024 at 10:32:54AM GMT, Daniel P. Berrangé wrote:
> On Mon, Aug 05, 2024 at 09:21:36PM -0500, Eric Blake wrote:
> > Since an NBD server may be long-living, serving clients that
> > repeatedly connect and disconnect, it can be more efficient to clean
> > up after each client disconnects, rather than storing a list of
> > resources to clean up when the server exits.  Rewrite the list of
> > known clients to be double-linked so that we can get O(1) deletion to
> > keep the list pruned to size as clients exit.  This in turn requires
> > each client to track an opaque pointer of owner information (although
> > qemu-nbd doesn't need to refer to it).
> 
> I tend to feel that this needs to be squashed into the previous
> patch.  The previous patch effectively creates unbounded memory
> usage in the NBD server. ie consider a client that connects and
> immediately disconnects, not sending any data, in a tight loop.
> It will "leak" NBDConn & QIOChanelSocket pointers for each
> iteration of the loop, only to be cleaned up when the NBD Server
> is shutdown.

Hmm. Offline, we observed that qemu's default of unlimited clients may
not be the smartest thing - if max-connections is not set, we may want
to default it to something like 100 (even when MULTI_CONN is
exploited, most clients that take advantage of it will probably only
use 8 or 16 parallel sockets; more than that tends to run into other
limits rather than providing continual improvements).  I can add such
a change in default in v4.  In contrast, qemu-nbd defaults to
max-connections 1 for historical reasons (among others, if you have a
non-persistent server, it is really nice that qemu-nbd automatically
exits after the first client disconnects, rather than needing to clean
up the server yourself), but that's too small for MULTI_CONN to be of
any use.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org