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 adds the low-level client code for grabbing the list > of exports. It benefits from the recent refactoring patches, as > well as a minor tweak of changing nbd_opt_go() to nbd_opt_info_or_go(), > in order to share as much code as possible when it comes to doing > validation of server replies. The resulting information is stored > in an array of NBDExportInfo which has been expanded to any > description string, along with a convenience function for freeing > the list. > > Note: a malicious server could exhaust memory of a client by feeding > an unending loop of exports; perhaps we should place a limit on how > many we are willing to receive. But note that a server could > reasonably be serving an export for every file in a large directory, > where an arbitrary limit in the client means we can't list anything > from such a server; the same happens if we just run until the client > fails to malloc() and thus dies by an abort(), where the limit is > no longer arbitrary but determined by available memory. Since the > client is already planning on being short-lived, it's hard to call > this a denial of service attack that would starve off other uses, > so it does not appear to be a security issue. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > Message-Id: <20181215135324.152629-19-ebl...@redhat.com> > Reviewed-by: Richard W.M. Jones <rjo...@redhat.com> > > --- > v3: mention security (non-)issue in commit message [Rich], formatting > tweaks > ---
[..] > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -330,11 +330,14 @@ static int nbd_receive_list(QIOChannel *ioc, char > **name, char **description, > } > > > -/* Returns -1 if NBD_OPT_GO proves the export @info->name cannot be > +/* > + * Returns -1 if NBD_OPT_GO proves the export @info->name cannot be > * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and > * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to > - * go (with the rest of @info populated). */ > -static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo *info, Error **errp) > + * go (with the rest of @info populated). > + */ Don't you want to upgrade a comment a little bit, to reflect support for OPT_INFO? > +static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt, > + NBDExportInfo *info, Error **errp) > { > NBDOptionReply reply; > uint32_t len = strlen(info->name); > @@ -347,7 +350,8 @@ static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo > *info, Error **errp) > * flags still 0 is a witness of a broken server. */ > info->flags = 0; > > - trace_nbd_opt_go_start(info->name); > + assert(opt == NBD_OPT_GO || opt == NBD_OPT_INFO); > + trace_nbd_opt_go_start(nbd_opt_lookup(opt), info->name); I'd prefer to upgrade trace-point name too, as well as other several trace_nbd_opt_go_* trace points in the function. > buf = g_malloc(4 + len + 2 + 2 * info->request_sizes + 1); > stl_be_p(buf, len); > memcpy(buf + 4, info->name, len); [..] > +/* > + * nbd_receive_export_list: > + * Query details about a server's exports, then disconnect without > + * going into transmission phase. Return a count of the exports listed > + * in @info by the server, or -1 on error. Caller must free @info using > + * nbd_free_export_list(). > + */ > +int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, > + const char *hostname, NBDExportInfo **info, > + Error **errp) > +{ > + int result; > + int count = 0; > + int i; > + int rc; > + int ret = -1; > + NBDExportInfo *array = NULL; > + QIOChannel *sioc = NULL; > + > + *info = NULL; > + result = nbd_start_negotiate(ioc, tlscreds, hostname, &sioc, true, NULL, > + errp); > + if (tlscreds && sioc) { > + ioc = sioc; > + } > + > + switch (result) { > + case 2: > + case 3: > + /* newstyle - use NBD_OPT_LIST to populate array, then try > + * NBD_OPT_INFO on each array member. If structured replies > + * are enabled, also try NBD_OPT_LIST_META_CONTEXT. */ > + if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) { > + goto out; > + } > + while (1) { > + char *name; > + char *desc; > + > + rc = nbd_receive_list(ioc, &name, &desc, errp); > + if (rc < 0) { > + goto out; > + } else if (rc == 0) { > + break; > + } > + array = g_renew(NBDExportInfo, array, ++count); > + memset(&array[count - 1], 0, sizeof(*array)); > + array[count - 1].name = name; > + array[count - 1].description = desc; > + array[count - 1].structured_reply = result == 3; > + } > + > + for (i = 0; i < count; i++) { > + array[i].request_sizes = true; > + rc = nbd_opt_info_or_go(ioc, NBD_OPT_INFO, &array[i], errp); > + if (rc < 0) { > + goto out; > + } else if (rc == 0) { > + /* Pointless to try rest of loop. If OPT_INFO doesn't work, > + * it's unlikely that meta contexts work either */ > + break; > + } > + > + /* TODO: Grab meta contexts */ > + } > + > + /* Send NBD_OPT_ABORT as a courtesy before hanging up */ > + nbd_send_opt_abort(ioc); > + break; > + case 1: /* newstyle, but limited to EXPORT_NAME */ > + error_setg(errp, "Server does not support export lists"); > + /* We can't even send NBD_OPT_ABORT, so merely hang up */ But, on the other hand, why not to send it? It will be unknown for the server, so, it will have to close the connection accordingly to the protocol, isn't it better a bit? > + goto out; > + case 0: /* oldstyle, parse length and flags */ > + array = g_new0(NBDExportInfo, 1); > + array->name = g_strdup(""); > + count = 1; > + > + if (nbd_negotiate_finish_oldstyle(ioc, array, errp) < 0) { > + return -EINVAL; goto out, you mean. And with at least this one fixed: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> -- Best regards, Vladimir