Message ID | 20190112175812.27068-17-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Series | None | expand |
12.01.2019 20:58, Eric Blake wrote: > We want to be able to detect whether a given qemu NBD server is > exposing the right export(s) and dirty bitmaps, at least for > regression testing. We could use 'nbd-client -l' from the upstream > NBD project to list exports, but it's annoying to rely on > out-of-tree binaries; furthermore, nbd-client doesn't necessarily > know about all of the qemu NBD extensions. Thus, we plan on adding > a new mode to qemu-nbd that merely sniffs all possible information > from the server during handshake phase, then disconnects and dumps > the information. > > This patch continues the work of the previous patch, by adding the > ability to track the list of available meta contexts into > NBDExportInfo. It benefits from the recent refactoring patches > with a new nbd_list_meta_contexts() that reuses much of the same > framework as setting a meta context. > > Note: a malicious server could exhaust memory of a client by feeding > an unending loop of contexts; perhaps we could place a limit on how > many we are willing to receive. But this is no different from our > earlier analysis on a server sending an unending list of exports, > and the death of a client due to memory exhaustion when the client > was going to exit soon anyways is not really a denial of service > attack. > > Signed-off-by: Eric Blake <eblake@redhat.com> > Message-Id: <20181215135324.152629-20-eblake@redhat.com> > Reviewed-by: Richard W.M. Jones <rjones@redhat.com> > > --- > v3: mention security (non-)issue in commit message [Rich] > --- > include/block/nbd.h | 2 ++ > nbd/client.c | 41 +++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 41 insertions(+), 2 deletions(-) > > diff --git a/include/block/nbd.h b/include/block/nbd.h > index e9a442ce7e9..80ee3cc997e 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -284,6 +284,8 @@ struct NBDExportInfo { > > /* Set by server results during nbd_receive_export_list() */ > char *description; > + int n_contexts; > + char **contexts; > }; > typedef struct NBDExportInfo NBDExportInfo; > > diff --git a/nbd/client.c b/nbd/client.c > index a5a705a67f7..2001e6e8160 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -817,6 +817,36 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > return received; > } > > +/* > + * nbd_list_meta_contexts: > + * Request the server to list all meta contexts for export @info->name. > + * return 0 if list is complete (even if empty), > + * -1 with errp set for any other error Not sure about need of "other" word in this context. > + */ > +static int nbd_list_meta_contexts(QIOChannel *ioc, > + NBDExportInfo *info, > + Error **errp) > +{ > + int ret; > + > + if (nbd_send_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT, > + info->name, NULL, errp) < 0) { Hmm, this made me to add one more comment to previous patch about nbd_send_one_meta_context, the it don't send context but query. > + return -1; > + } > + > + while (1) { > + char *context; > + > + ret = nbd_receive_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT, > + &context, NULL, errp); > + if (ret <= 0) { > + return ret; > + } > + info->contexts = g_renew(char *, info->contexts, ++info->n_contexts); > + info->contexts[info->n_contexts - 1] = context; > + } A bit odd for me, that function leaves allocated memory on error path, but it will be freed anyway in nbd_free_export_list, so, don't mind. > +} > + > /* > * nbd_start_negotiate: > * Start the handshake to the server. After a positive return, the server > @@ -1063,7 +1093,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, > /* Clean up result of nbd_receive_export_list */ > void nbd_free_export_list(NBDExportInfo *info, int count) > { > - int i; > + int i, j; > > if (!info) { > return; > @@ -1072,6 +1102,10 @@ void nbd_free_export_list(NBDExportInfo *info, int count) > for (i = 0; i < count; i++) { > g_free(info[i].name); > g_free(info[i].description); > + for (j = 0; j < info[i].n_contexts; j++) { > + g_free(info[i].contexts[j]); > + } > + g_free(info[i].contexts); > } > g_free(info); > } > @@ -1139,7 +1173,10 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, > break; > } > > - /* TODO: Grab meta contexts */ > + if (result == 3 && > + nbd_list_meta_contexts(ioc, &array[i], errp) < 0) { > + goto out; > + } > } > > /* Send NBD_OPT_ABORT as a courtesy before hanging up */ > anyway, Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
diff --git a/include/block/nbd.h b/include/block/nbd.h index e9a442ce7e9..80ee3cc997e 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -284,6 +284,8 @@ struct NBDExportInfo { /* Set by server results during nbd_receive_export_list() */ char *description; + int n_contexts; + char **contexts; }; typedef struct NBDExportInfo NBDExportInfo; diff --git a/nbd/client.c b/nbd/client.c index a5a705a67f7..2001e6e8160 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -817,6 +817,36 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, return received; } +/* + * nbd_list_meta_contexts: + * Request the server to list all meta contexts for export @info->name. + * return 0 if list is complete (even if empty), + * -1 with errp set for any other error + */ +static int nbd_list_meta_contexts(QIOChannel *ioc, + NBDExportInfo *info, + Error **errp) +{ + int ret; + + if (nbd_send_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT, + info->name, NULL, errp) < 0) { + return -1; + } + + while (1) { + char *context; + + ret = nbd_receive_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT, + &context, NULL, errp); + if (ret <= 0) { + return ret; + } + info->contexts = g_renew(char *, info->contexts, ++info->n_contexts); + info->contexts[info->n_contexts - 1] = context; + } +} + /* * nbd_start_negotiate: * Start the handshake to the server. After a positive return, the server @@ -1063,7 +1093,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, /* Clean up result of nbd_receive_export_list */ void nbd_free_export_list(NBDExportInfo *info, int count) { - int i; + int i, j; if (!info) { return; @@ -1072,6 +1102,10 @@ void nbd_free_export_list(NBDExportInfo *info, int count) for (i = 0; i < count; i++) { g_free(info[i].name); g_free(info[i].description); + for (j = 0; j < info[i].n_contexts; j++) { + g_free(info[i].contexts[j]); + } + g_free(info[i].contexts); } g_free(info); } @@ -1139,7 +1173,10 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, break; } - /* TODO: Grab meta contexts */ + if (result == 3 && + nbd_list_meta_contexts(ioc, &array[i], errp) < 0) { + goto out; + } } /* Send NBD_OPT_ABORT as a courtesy before hanging up */