Re: [Qemu-devel] [PATCH v3 16/19] nbd/client: Add meta contexts to nbd_receive_export_list()

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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>


-- 
Best regards,
Vladimir