Re: [Qemu-block] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT

[Vladimir Sementsov-Ogievskiy]

30.03.2018 02:18, Eric Blake wrote:
> It's never a good idea to blindly read for size bytes as
> returned by the server without first validating that the size
> is within bounds; a malicious or buggy server could cause us
> to hang or get out of sync from reading further messages.
>
> It may be smarter to try and teach the client to cope with
> unexpected context ids by silently ignoring them instead of
> hanging up on the server, but for now, if the server doesn't
> reply with exactly the one context we expect, it's easier to
> just give up - however, if we give up for any reason other
> than an I/O failure, we might as well try to politely tell
> the server we are quitting rather than continuing.
>
> Fix some typos in the process.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   nbd/client.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 9b9b7f0ea29..4ee1d9a4a2c 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -599,8 +599,8 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
>    * Set one meta context. Simple means that reply must contain zero (not
>    * negotiated) or one (negotiated) contexts. More contexts would be considered
>    * as a protocol error. It's also implied that meta-data query equals queried
> - * context name, so, if server replies with something different then @context,
> - * it considered as error too.
> + * context name, so, if server replies with something different than @context,
> + * it is considered an error too.
>    * return 1 for successful negotiation, context_id is set
>    *        0 if operation is unsupported,
>    *        -1 with errp set for any other error
> @@ -651,6 +651,14 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>           char *name;
>           size_t len;
>
> +        if (reply.length != sizeof(received_id) + context_len) {
> +            error_setg(errp, "Failed to negotiate meta context '%s', server "
> +                       "answered with unexpected length %u", context,

uint32_t, is it worth PRIu32 ? Or %u is absolutely portable in this case?

> +                       reply.length);
> +            nbd_send_opt_abort(ioc);
> +            return -1;
> +        }

hmm, after this check, len variable is not actually needed, we can use 
context_len

> +
>           if (nbd_read(ioc, &received_id, sizeof(received_id), errp) < 0) {
>               return -1;
>           }
> @@ -668,6 +676,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>                          "answered with different context '%s'", context,
>                          name);
>               g_free(name);
> +            nbd_send_opt_abort(ioc);
>               return -1;
>           }
>           g_free(name);
> @@ -690,6 +699,12 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>       if (reply.type != NBD_REP_ACK) {
>           error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
>                      reply.type, NBD_REP_ACK);
> +        nbd_send_opt_abort(ioc);
> +        return -1;
> +    }
> +    if (reply.length) {

this check is very common for REP_ACK, it may be better to move it to 
nbd_handle_reply_err... (and rename this function? and  combine it 
somehow with _option_request() and _option_reply()?)

> +        error_setg(errp, "Unexpected length to ACK response");
> +        nbd_send_opt_abort(ioc);

hmm, looks like we want nbd_send_opt_abort() before most of return -1. 
Looks like it lacks some generalization, may be want to send it at some 
common point..

>           return -1;
>       }

mostly, just ideas for future refactoring, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

--
Best regards,
Vladimir