[12/18] nbd: Less allocation during NBD_OPT_LIST

Since we know that the maximum name we are willing to accept
is small enough to stack-allocate, rework the iteration over
NBD_OPT_LIST responses to reuse a stack buffer rather than
allocating every time.  Furthermore, we don't even have to
allocate if we know the server's length doesn't match what
we are searching for.

Not fixed here: Upstream NBD Protocol recently added this
clarification:
https://github.com/yoe/nbd/blob/18918eb/doc/proto.md#conventions

 Where this document refers to a string, then unless otherwise
 stated, that string is a sequence of UTF-8 code points, which
 is not NUL terminated, MUST NOT contain NUL characters, SHOULD
 be no longer than 256 bytes and MUST be no longer than 4096
 bytes. This applies to export names and error messages (amongst
 others).

To be fully compliant to that, we need to bump our export name
limit from 255 to at least 256, and need to decide whether we
can bump it higher (bumping it all the way to 4096 is annoying
in that we could no longer safely stack-allocate a worst-case
string, so we may still want to take the leeway offered by SHOULD
to force a reasonable smaller limit).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/client.c | 130 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 65 insertions(+), 65 deletions(-)

On 8 Apr 2016, at 23:05, Eric Blake <eblake@redhat.com> wrote:

> Since we know that the maximum name we are willing to accept
> is small enough to stack-allocate, rework the iteration over
> NBD_OPT_LIST responses to reuse a stack buffer rather than
> allocating every time.  Furthermore, we don't even have to
> allocate if we know the server's length doesn't match what
> we are searching for.
> 
> Not fixed here: Upstream NBD Protocol recently added this
> clarification:
> https://github.com/yoe/nbd/blob/18918eb/doc/proto.md#conventions
> 
> Where this document refers to a string, then unless otherwise
> stated, that string is a sequence of UTF-8 code points, which
> is not NUL terminated, MUST NOT contain NUL characters, SHOULD
> be no longer than 256 bytes and MUST be no longer than 4096
> bytes. This applies to export names and error messages (amongst
> others).
> 
> To be fully compliant to that, we need to bump our export name
> limit from 255 to at least 256, and need to decide whether we
> can bump it higher (bumping it all the way to 4096 is annoying
> in that we could no longer safely stack-allocate a worst-case
> string, so we may still want to take the leeway offered by SHOULD
> to force a reasonable smaller limit).

Is there a limit in qemu-world to safe stack allocation? I thought
that was (in general) only a kernel consideration? (probably my
ignorance here).

Otherwise:


Reviewed-by: Alex Bligh <alex@alex.org.uk>

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> nbd/client.c | 130 +++++++++++++++++++++++++++++------------------------------
> 1 file changed, 65 insertions(+), 65 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index b2dfc11..d4e37d5 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -230,14 +230,17 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
>     return result;
> }
> 
> -static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
> +/* Return -1 if unrecoverable error occurs, 0 if NBD_OPT_LIST is
> + * unsupported, 1 if iteration is done, 2 to keep looking, and 3 if
> + * this entry matches want. */
> +static int nbd_receive_list(QIOChannel *ioc, const char *want, Error **errp)
> {
>     nbd_opt_reply reply;
>     uint32_t len;
>     uint32_t namelen;
> +    char name[NBD_MAX_NAME_SIZE + 1];
>     int error;
> 
> -    *name = NULL;
>     if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
>         return -1;
>     }
> @@ -252,97 +255,94 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
>             error_setg(errp, "length too long for option end");
>             return -1;
>         }
> -    } else if (reply.type == NBD_REP_SERVER) {
> -        if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
> -            error_setg(errp, "incorrect option length %"PRIu32, len);
> -            return -1;
> -        }
> -        if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
> -            error_setg(errp, "failed to read option name length");
> -            return -1;
> -        }
> -        namelen = be32_to_cpu(namelen);
> -        len -= sizeof(namelen);
> -        if (len < namelen) {
> -            error_setg(errp, "incorrect option name length");
> -            return -1;
> -        }
> -        if (namelen > NBD_MAX_NAME_SIZE) {
> -            error_setg(errp, "export name length too long %" PRIu32, namelen);
> -            return -1;
> -        }
> -
> -        *name = g_new0(char, namelen + 1);
> -        if (read_sync(ioc, *name, namelen) != namelen) {
> -            error_setg(errp, "failed to read export name");
> -            g_free(*name);
> -            *name = NULL;
> -            return -1;
> -        }
> -        (*name)[namelen] = '\0';
> -        len -= namelen;
> -        if (drop_sync(ioc, len) != len) {
> -            error_setg(errp, "failed to read export description");
> -            g_free(*name);
> -            *name = NULL;
> -            return -1;
> -        }
> -    } else {
> +        return 1;
> +    } else if (reply.type != NBD_REP_SERVER) {
>         error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
>                    reply.type, NBD_REP_SERVER);
>         return -1;
>     }
> -    return 1;
> +
> +    if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
> +        error_setg(errp, "incorrect option length %"PRIu32, len);
> +        return -1;
> +    }
> +    if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
> +        error_setg(errp, "failed to read option name length");
> +        return -1;
> +    }
> +    namelen = be32_to_cpu(namelen);
> +    len -= sizeof(namelen);
> +    if (len < namelen) {
> +        error_setg(errp, "incorrect option name length");
> +        return -1;
> +    }
> +    if (namelen != strlen(want)) {
> +        if (drop_sync(ioc, len) != len) {
> +            error_setg(errp, "failed to skip export name with wrong length");
> +            return -1;
> +        }
> +        return 2;
> +    }
> +
> +    assert(namelen < sizeof(name));
> +    if (read_sync(ioc, name, namelen) != namelen) {
> +        error_setg(errp, "failed to read export name");
> +        return -1;
> +    }
> +    name[namelen] = '\0';
> +    len -= namelen;
> +    if (drop_sync(ioc, len) != len) {
> +        error_setg(errp, "failed to read export description");
> +        return -1;
> +    }
> +    return strcmp(name, want) == 0 ? 3 : 2;
> }
> 
> 
> +/* Return -1 on failure, 0 if wantname is an available export. */
> static int nbd_receive_query_exports(QIOChannel *ioc,
>                                      const char *wantname,
>                                      Error **errp)
> {
>     bool foundExport = false;
> 
> -    TRACE("Querying export list");
> +    TRACE("Querying export list for '%s'", wantname);
>     if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) {
>         return -1;
>     }
> 
>     TRACE("Reading available export names");
>     while (1) {
> -        char *name = NULL;
> -        int ret = nbd_receive_list(ioc, &name, errp);
> +        int ret = nbd_receive_list(ioc, wantname, errp);
> 
> -        if (ret < 0) {
> -            g_free(name);
> -            name = NULL;
> +        switch (ret) {
> +        default:
> +            /* Server gave unexpected reply */
> +            assert(ret < 0);
>             return -1;
> -        }
> -        if (ret == 0) {
> +        case 0:
>             /* Server doesn't support export listing, so
>              * we will just assume an export with our
>              * wanted name exists */
> -            foundExport = true;
> -            break;
> -        }
> -        if (name == NULL) {
> -            TRACE("End of export name list");
> +            return 0;
> +        case 1:
> +            /* Done iterating. */
> +            if (!foundExport) {
> +                error_setg(errp, "No export with name '%s' available",
> +                           wantname);
> +                return -1;
> +            }
> +            return 0;
> +        case 2:
> +            /* Wasn't this one, keep going. */
>             break;
> -        }
> -        if (g_str_equal(name, wantname)) {
> +        case 3:
> +            /* Found a match, but must finish parsing reply. */
> +            TRACE("Found desired export name '%s'", wantname);
>             foundExport = true;
> -            TRACE("Found desired export name '%s'", name);
> -        } else {
> -            TRACE("Ignored export name '%s'", name);
> +            break;
>         }
> -        g_free(name);
> -    }
> -
> -    if (!foundExport) {
> -        error_setg(errp, "No export with name '%s' available", wantname);
> -        return -1;
>     }
> -
> -    return 0;
> }
> 
> static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> -- 
> 2.5.5
> 
>

On 04/09/2016 04:41 AM, Alex Bligh wrote:
> 
> On 8 Apr 2016, at 23:05, Eric Blake <eblake@redhat.com> wrote:
> 
>> Since we know that the maximum name we are willing to accept
>> is small enough to stack-allocate, rework the iteration over
>> NBD_OPT_LIST responses to reuse a stack buffer rather than
>> allocating every time.  Furthermore, we don't even have to
>> allocate if we know the server's length doesn't match what
>> we are searching for.
>>
>> Not fixed here: Upstream NBD Protocol recently added this
>> clarification:
>> https://github.com/yoe/nbd/blob/18918eb/doc/proto.md#conventions
>>
>> Where this document refers to a string, then unless otherwise
>> stated, that string is a sequence of UTF-8 code points, which
>> is not NUL terminated, MUST NOT contain NUL characters, SHOULD
>> be no longer than 256 bytes and MUST be no longer than 4096
>> bytes. This applies to export names and error messages (amongst
>> others).
>>
>> To be fully compliant to that, we need to bump our export name
>> limit from 255 to at least 256, and need to decide whether we
>> can bump it higher (bumping it all the way to 4096 is annoying
>> in that we could no longer safely stack-allocate a worst-case
>> string, so we may still want to take the leeway offered by SHOULD
>> to force a reasonable smaller limit).
> 
> Is there a limit in qemu-world to safe stack allocation? I thought
> that was (in general) only a kernel consideration? (probably my
> ignorance here).

Even in user space apps, any stack allocation larger than 4096 bytes
risks skipping over the guard page on Windows, which makes the
difference in whether you get a SIGSEGV (good) or a hard process kill
(bad).  We're slowly getting qemu to the point where it will compile
with gcc's options go guarantee that no one function requires more than
4k stack.

> 
> Otherwise:
> 
> 
> Reviewed-by: Alex Bligh <alex@alex.org.uk>
> 

And continuing from the things I mentioned in the other mail regarding
export name limits...

>> -    return 1;
>> +
>> +    if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
>> +        error_setg(errp, "incorrect option length %"PRIu32, len);
>> +        return -1;
>> +    }

This is a case of a faulty server stream (whether evil server, or MitM,
or whatever else...); if the packet wasn't big enough to include
namelen, or if the message size is larger than 32M, the stream is
considered corrupt to the point that it is no longer worth talking to
the server (hence, the return -1).

>> +    if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
>> +        error_setg(errp, "failed to read option name length");
>> +        return -1;
>> +    }

Likewise, anywhere we fail to read the server stream (most often due to
stream disconnect causing EOF), returning -1 is fine because we can't
recover anyway.

>> +    namelen = be32_to_cpu(namelen);
>> +    len -= sizeof(namelen);
>> +    if (len < namelen) {
>> +        error_setg(errp, "incorrect option name length");
>> +        return -1;
>> +    }

Likewise, if the server gives a namelen that would read beyond the
bounds of the overall packet length, the server can't be trusted for
anything else.

>> +    if (namelen != strlen(want)) {
>> +        if (drop_sync(ioc, len) != len) {
>> +            error_setg(errp, "failed to skip export name with wrong length");
>> +            return -1;
>> +        }
>> +        return 2;
>> +    }

While this gracefully handles any remaining string size, even up to
qemu's NBD_MAX_BUFFER_SIZE (32M), well beyond the required 256 or
recommended 4096 of the protocol.

>> +
>> +    assert(namelen < sizeof(name));
>> +    if (read_sync(ioc, name, namelen) != namelen) {
>> +        error_setg(errp, "failed to read export name");
>> +        return -1;
>> +    }
>> +    name[namelen] = '\0';
>> +    len -= namelen;
>> +    if (drop_sync(ioc, len) != len) {
>> +        error_setg(errp, "failed to read export description");
>> +        return -1;
>> +    }
>> +    return strcmp(name, want) == 0 ? 3 : 2;

So with this patch, I've worked it into allowing any string sizes from
the server, while focusing only on the strings that match the length of
the name requested by the client; now the audit proceeds to find out
whether letting the client request a name longer than 255 makes sense,
but at least this part of the client/server interaction is safe, and
ready for a later patch to bump the size of the #define for max name
length, unless bumping it too large makes the local buf[] exceed
preferred stack size.

[It's also interesting to note that once the NBD_OPT_GO code is live and
supported by more servers, we won't even be hitting this section of
NBD_OPT_INFO code in the qemu client]