Re: [Qemu-devel] [PATCH v2 12/22] nbd/client: Improve error handling in nbd_negotiate_simple_meta_context()

[Richard W.M. Jones]

On Sat, Dec 15, 2018 at 07:53:14AM -0600, Eric Blake wrote:
> Always allocate space for the reply returned by the server and
> hoist the trace earlier, as it is more interesting to trace the
> server's reply (even if it is unexpected) than parroting our
> request only on success.  After all, skipping the allocation
> for a wrong size was merely a micro-optimization that only
> benefitted a broken server, rather than the common case of a
> compliant server that meets our expectations.
> 
> Then turn the reply handling into a loop (even though we still
> never iterate more than once), to make this code easier to use
> when later patches do support multiple server replies.  This
> changes the error message for a server with two replies (a
> corner case we are unlikely to hit in practice) from:
> 
> Unexpected reply type 4 (meta context), expected 0 (ack)
> 
> to:
> 
> Server replied with more than one context
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v2: split patch into easier-to-review pieces [Rich, Vladimir]
> ---
>  nbd/client.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index bcccd5f555e..b6a85fc3ef8 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -684,10 +684,11 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>          return ret;
>      }
> 
> -    if (reply.type == NBD_REP_META_CONTEXT) {
> +    while (reply.type == NBD_REP_META_CONTEXT) {

I'm not sure I understand why this change is safe.

As far as I can see reply.type is only updated in the loop by
nbd_receive_option_reply, and that reads from the server, and so the
server might keep sending NBD_REP_META_CONTEXT packets (instead of the
expected NBD_REP_ACK), so it could now loop forever against a
malicious server?  (This is not taking into account any later patches)

Rich.

>          char *name;
> 
> -        if (reply.length != sizeof(info->context_id) + context_len) {
> +        if (reply.length <= sizeof(info->context_id) ||
> +            reply.length > NBD_MAX_BUFFER_SIZE) {
>              error_setg(errp, "Failed to negotiate meta context '%s', server "
>                         "answered with unexpected length %" PRIu32, context,
>                         reply.length);
> @@ -708,6 +709,15 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>              return -1;
>          }
>          name[reply.length] = '\0';
> +        trace_nbd_opt_meta_reply(name, info->context_id);
> +
> +        if (received) {
> +            error_setg(errp, "Server replied with more than one context");
> +            g_free(name);
> +            nbd_send_opt_abort(ioc);
> +            return -1;
> +        }
> +
>          if (strcmp(context, name)) {
>              error_setg(errp, "Failed to negotiate meta context '%s', server "
>                         "answered with different context '%s'", context,
> @@ -717,8 +727,6 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>              return -1;
>          }
>          g_free(name);
> -
> -        trace_nbd_opt_meta_reply(context, info->context_id);
>          received = true;
> 
>          /* receive NBD_REP_ACK */
> -- 
> 2.17.2

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW