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

2018-04-02 Thread 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(_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 

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

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

2018-03-30 Thread 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 
---
  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, _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 

--
Best regards,
Vladimir

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

2018-03-29 Thread Eric Blake 
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 
---
 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,
+   reply.length);
+nbd_send_opt_abort(ioc);
+return -1;
+}
+
 if (nbd_read(ioc, _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) {
+error_setg(errp, "Unexpected length to ACK response");
+nbd_send_opt_abort(ioc);
 return -1;
 }

-- 
2.14.3