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
+
/* 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)
--
2.45.2On 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
--
|: 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 :|
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
© 2016 - 2024 Red Hat, Inc.