diff mbox series

[v4,2/7] nbd/server: Plumb in new args to nbd_client_add()

Message ID 20240807174943.771624-11-eblake@redhat.com (mailing list archive)
State New, archived
Headers show
Series CVE-2024-7409 | expand

Commit Message

Eric Blake Aug. 7, 2024, 5:43 p.m. UTC 
Upcoming patches to fix a CVE need to track an opaque pointer passed
in by the owner of a client object, as well as reequest for a time
limit on how fast negotiation must complete.  Prepare for that by
changing the signature of nbd_client_new() and adding an accessor to
get at the opaque pointer, although for now the two servers
(qemu-nbd.c and blockdev-nbd.c) do not change behavior.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h | 11 ++++++++++-
 blockdev-nbd.c      |  6 ++++--
 nbd/server.c        | 20 +++++++++++++++++---
 qemu-nbd.c          |  4 +++-
 4 files changed, 34 insertions(+), 7 deletions(-)

Comments

Daniel P. Berrangé Aug. 7, 2024, 5:58 p.m. UTC | #1 
On Wed, Aug 07, 2024 at 12:43:28PM -0500, Eric Blake wrote:
> Upcoming patches to fix a CVE need to track an opaque pointer passed
> in by the owner of a client object, as well as reequest for a time
> limit on how fast negotiation must complete.  Prepare for that by
> changing the signature of nbd_client_new() and adding an accessor to
> get at the opaque pointer, although for now the two servers
> (qemu-nbd.c and blockdev-nbd.c) do not change behavior.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/nbd.h | 11 ++++++++++-
>  blockdev-nbd.c      |  6 ++++--
>  nbd/server.c        | 20 +++++++++++++++++---
>  qemu-nbd.c          |  4 +++-
>  4 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 4e7bd6342f9..5fe14786414 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -33,6 +33,12 @@ typedef struct NBDMetaContexts NBDMetaContexts;
> 
>  extern const BlockExportDriver blk_exp_nbd;
> 
> +/*
> + * NBD_DEFAULT_HANDSHAKE_LIMIT: Number of seconds in which client must
> + * succeed at NBD_OPT_GO before being forcefully dropped as too slow.
> + */
> +#define NBD_DEFAULT_HANDSHAKE_LIMIT 10

Suggest

s/NBD_DEFAULT_HANDSHAKE_LIMIT/NBD_DEFAULT_HANDSHAKE_MAX_SECS/


> +
>  /* Handshake phase structs - this struct is passed on the wire */
> 
>  typedef struct NBDOption {
> @@ -403,9 +409,12 @@ AioContext *nbd_export_aio_context(NBDExport *exp);
>  NBDExport *nbd_export_find(const char *name);
> 
>  void nbd_client_new(QIOChannelSocket *sioc,
> +                    uint32_t handshake_limit,

s/handshake_limit/handshake_max_secs/

to make the units of the parameter self-documenting.

Since this is a non-functional suggestion

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


With regards,
Daniel
Eric Blake Aug. 7, 2024, 9 p.m. UTC | #2 
On Wed, Aug 07, 2024 at 06:58:36PM GMT, Daniel P. Berrangé wrote:
> On Wed, Aug 07, 2024 at 12:43:28PM -0500, Eric Blake wrote:
> > Upcoming patches to fix a CVE need to track an opaque pointer passed
> > in by the owner of a client object, as well as reequest for a time

s/reequest/request/

> > limit on how fast negotiation must complete.  Prepare for that by
> > changing the signature of nbd_client_new() and adding an accessor to
> > get at the opaque pointer, although for now the two servers
> > (qemu-nbd.c and blockdev-nbd.c) do not change behavior.
> > 
> > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  include/block/nbd.h | 11 ++++++++++-
> >  blockdev-nbd.c      |  6 ++++--
> >  nbd/server.c        | 20 +++++++++++++++++---
> >  qemu-nbd.c          |  4 +++-
> >  4 files changed, 34 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/block/nbd.h b/include/block/nbd.h
> > index 4e7bd6342f9..5fe14786414 100644
> > --- a/include/block/nbd.h
> > +++ b/include/block/nbd.h
> > @@ -33,6 +33,12 @@ typedef struct NBDMetaContexts NBDMetaContexts;
> > 
> >  extern const BlockExportDriver blk_exp_nbd;
> > 
> > +/*
> > + * NBD_DEFAULT_HANDSHAKE_LIMIT: Number of seconds in which client must
> > + * succeed at NBD_OPT_GO before being forcefully dropped as too slow.
> > + */
> > +#define NBD_DEFAULT_HANDSHAKE_LIMIT 10
> 
> Suggest
> 
> s/NBD_DEFAULT_HANDSHAKE_LIMIT/NBD_DEFAULT_HANDSHAKE_MAX_SECS/

I like it.

> 
> 
> > +
> >  /* Handshake phase structs - this struct is passed on the wire */
> > 
> >  typedef struct NBDOption {
> > @@ -403,9 +409,12 @@ AioContext *nbd_export_aio_context(NBDExport *exp);
> >  NBDExport *nbd_export_find(const char *name);
> > 
> >  void nbd_client_new(QIOChannelSocket *sioc,
> > +                    uint32_t handshake_limit,
> 
> s/handshake_limit/handshake_max_secs/
> 
> to make the units of the parameter self-documenting.
> 
> Since this is a non-functional suggestion
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Will adjust the series with the fallout.

At this point, I'm leaning towards queuing 1-5 in an upcoming pull
request, but leaving 6-7 for the 9.2 development cycle.
diff mbox series

Patch

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4e7bd6342f9..5fe14786414 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -33,6 +33,12 @@  typedef struct NBDMetaContexts NBDMetaContexts;

 extern const BlockExportDriver blk_exp_nbd;

+/*
+ * NBD_DEFAULT_HANDSHAKE_LIMIT: Number of seconds in which client must
+ * succeed at NBD_OPT_GO before being forcefully dropped as too slow.
+ */
+#define NBD_DEFAULT_HANDSHAKE_LIMIT 10
+
 /* Handshake phase structs - this struct is passed on the wire */

 typedef struct NBDOption {
@@ -403,9 +409,12 @@  AioContext *nbd_export_aio_context(NBDExport *exp);
 NBDExport *nbd_export_find(const char *name);

 void nbd_client_new(QIOChannelSocket *sioc,
+                    uint32_t handshake_limit,
                     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 213012435f4..11f878b6db3 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -64,8 +64,10 @@  static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
     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);
+    /* TODO - expose handshake limit as QMP option */
+    nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_LIMIT,
+                   nbd_server->tlscreds, nbd_server->tlsauthz,
+                   nbd_blockdev_client_closed, NULL);
 }

 static void nbd_update_server_watch(NBDServerData *s)
diff --git a/nbd/server.c b/nbd/server.c
index ecd9366ba64..31b77bf0d4f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -124,12 +124,14 @@  struct NBDMetaContexts {
 struct NBDClient {
     int refcount; /* atomic */
     void (*close_fn)(NBDClient *client, bool negotiated);
+    void *owner;

     QemuMutex lock;

     NBDExport *exp;
     QCryptoTLSCreds *tlscreds;
     char *tlsauthz;
+    uint32_t handshake_limit;
     QIOChannelSocket *sioc; /* The underlying data channel */
     QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */

@@ -3191,6 +3193,7 @@  static coroutine_fn void nbd_co_client_start(void *opaque)

     qemu_co_mutex_init(&client->send_lock);

+    /* TODO - utilize client->handshake_limit */
     if (nbd_negotiate(client, &local_err)) {
         if (local_err) {
             error_report_err(local_err);
@@ -3205,14 +3208,17 @@  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 with an indication of whether the client completed negotiation
+ * within @handshake_limit seconds (0 for unbounded).
  */
 void nbd_client_new(QIOChannelSocket *sioc,
+                    uint32_t handshake_limit,
                     QCryptoTLSCreds *tlscreds,
                     const char *tlsauthz,
-                    void (*close_fn)(NBDClient *, bool))
+                    void (*close_fn)(NBDClient *, bool),
+                    void *owner)
 {
     NBDClient *client;
     Coroutine *co;
@@ -3225,13 +3231,21 @@  void nbd_client_new(QIOChannelSocket *sioc,
         object_ref(OBJECT(client->tlscreds));
     }
     client->tlsauthz = g_strdup(tlsauthz);
+    client->handshake_limit = handshake_limit;
     client->sioc = sioc;
     qio_channel_set_delay(QIO_CHANNEL(sioc), false);
     object_ref(OBJECT(client->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 8e104ef22c3..7bf86a6566b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -390,7 +390,9 @@  static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,

     nb_fds++;
     nbd_update_server_watch();
-    nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed);
+    /* TODO - expose handshake limit as command line option */
+    nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_LIMIT,
+                   tlscreds, tlsauthz, nbd_client_closed, NULL);
 }

 static void nbd_update_server_watch(void)