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

[Eric Blake]

On 03/30/2018 11:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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.

>> @@ -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?

For trace-events, casting uint32_t to unsigned int is always safe, at
which point using %u is less typing (because the trace goes through a
function prototype conversion).  But when directly printing a uint32_t,
you are correct that some oddball 32-bit platforms might have uint32_t
be long, which would then trigger needless warnings if we don't use
PRIu32.  So I'll fix that.

> 
>> +                       reply.length);
>> +            nbd_send_opt_abort(ioc);
>> +            return -1;
>> +        }
> 
> hmm, after this check, len variable is not actually needed, we can use
> context_len
> 

Okay, I'm squashing this in:

diff --git i/nbd/client.c w/nbd/client.c
index 4ee1d9a4a2c..dd0174b036e 100644
--- i/nbd/client.c
+++ w/nbd/client.c
@@ -649,11 +649,10 @@ static int
nbd_negotiate_simple_meta_context(QIOChannel *ioc,

     if (reply.type == NBD_REP_META_CONTEXT) {
         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,
+                       "answered with unexpected length %" PRIu32, context,
                        reply.length);
             nbd_send_opt_abort(ioc);
             return -1;
@@ -664,13 +663,13 @@ static int
nbd_negotiate_simple_meta_context(QIOChannel *ioc,
         }
         be32_to_cpus(&received_id);

-        len = reply.length - sizeof(received_id);
-        name = g_malloc(len + 1);
-        if (nbd_read(ioc, name, len, errp) < 0) {
+        reply.length -= sizeof(received_id);
+        name = g_malloc(reply.length + 1);
+        if (nbd_read(ioc, name, reply.length, errp) < 0) {
             g_free(name);
             return -1;
         }
-        name[len] = '\0';
+        name[reply.length] = '\0';
         if (strcmp(context, name)) {
             error_setg(errp, "Failed to negotiate meta context '%s',
server "
                        "answered with different context '%s'", context,


>> @@ -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:

Indeed, any refactoring we do in that area belongs in 2.13 patches.

> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

Thanks; I'm including this in my NBD pull request today.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature