| Message ID | 20240807174943.771624-11-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | CVE-2024-7409 | expand |
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
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 --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)
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(-)