Re: [Qemu-devel] [PATCH 12/18] nbd: Less allocation during NBD_OPT_LIST

[Eric Blake]

On 04/09/2016 04:41 AM, Alex Bligh wrote:
> 
> On 8 Apr 2016, at 23:05, Eric Blake <address@hidden> 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 <address@hidden>
> 

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]

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature