Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Paolo Bonzini]

On 12/05/2016 00:39, Eric Blake wrote:
> We have a few bugs in how we handle invalid client commands:
> 
> - A client can send an NBD_CMD_DISC where from + len overflows,
> convincing us to reply with an error and stay connected, even
> though the protocol requires us to silently disconnect. Fix by
> hoisting the special case sooner.
> 
> - A client can send an NBD_CMD_WRITE with bogus from and len,
> where we reply to the client with EINVAL without consuming the
> payload; this will normally cause us to fail if the next thing
> read is not the right magic, but in rare cases, could cause us
> to interpret the data payload as valid commands and do things
> not requested by the client. Fix by adding a complete flag to
> track whether we are in sync or must disconnect.
> 
> - If we report an error to NBD_CMD_READ, we are not writing out
> any data payload; but the protocol says that a client can expect
> to read the payload no matter what (and must instead ignore it),
> which means the client will start reading our next replies as
> its data payload. Fix by disconnecting (an alternative fix of
> sending bogus payload would be trickier to implement).
> 
> Furthermore, we have split the checks for bogus from/len across
> two functions, when it is easier to do it all at once.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  nbd/server.c | 67 
> +++++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 46 insertions(+), 21 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 53507c5..9ac7e01 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -52,6 +52,7 @@ struct NBDRequest {
>      QSIMPLEQ_ENTRY(NBDRequest) entry;
>      NBDClient *client;
>      uint8_t *data;
> +    bool complete;
>  };
> 
>  struct NBDExport {
> @@ -989,7 +990,13 @@ static ssize_t nbd_co_send_reply(NBDRequest *req, struct 
> nbd_reply *reply,
>      return rc;
>  }
> 
> -static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request 
> *request)
> +/* Collect a client request.  Return 0 if request looks valid, -EAGAIN
> + * to keep trying the collection, -EIO to drop connection right away,
> + * and any other negative value to report an error to the client
> + * (although the caller may still need to disconnect after reporting
> + * the error).  */
> +static ssize_t nbd_co_receive_request(NBDRequest *req,
> +                                      struct nbd_request *request)
>  {
>      NBDClient *client = req->client;
>      uint32_t command;
> @@ -1007,16 +1014,26 @@ static ssize_t nbd_co_receive_request(NBDRequest 
> *req, struct nbd_request *reque
>          goto out;
>      }
> 
> -    if ((request->from + request->len) < request->from) {
> -        LOG("integer overflow detected! "
> -            "you're probably being attacked");
> -        rc = -EINVAL;
> -        goto out;
> -    }
> -
>      TRACE("Decoding type");
> 
>      command = request->type & NBD_CMD_MASK_COMMAND;
> +    if (command == NBD_CMD_DISC) {
> +        /* Special case: we're going to disconnect without a reply,
> +         * whether or not flags, from, or len are bogus */
> +        TRACE("Request type is DISCONNECT");
> +        rc = -EIO;
> +        goto out;
> +    }
> +
> +    /* Check for sanity in the parameters, part 1.  Defer as many
> +     * checks as possible until after reading any NBD_CMD_WRITE
> +     * payload, so we can try and keep the connection alive.  */
> +    if ((request->from + request->len) < request->from) {
> +        LOG("integer overflow detected, you're probably being attacked");
> +        rc = -EINVAL;
> +        goto out;
> +    }
> +
>      if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
>          if (request->len > NBD_MAX_BUFFER_SIZE) {
>              LOG("len (%" PRIu32" ) is larger than max len (%u)",
> @@ -1039,7 +1056,18 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, 
> struct nbd_request *reque
>              rc = -EIO;
>              goto out;
>          }
> +        req->complete = true;
>      }
> +
> +    /* Sanity checks, part 2. */
> +    if (request->from + request->len > client->exp->size) {
> +        LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
> +            ", Size: %" PRIu64, request->from, request->len,
> +            (uint64_t)client->exp->size);
> +        rc = -EINVAL;

For writes, this should be ENOSPC according to the spec.

> +        goto out;
> +    }
> +
>      rc = 0;
> 
>  out:
> @@ -1082,14 +1110,6 @@ static void nbd_trip(void *opaque)
>          goto error_reply;
>      }
>      command = request.type & NBD_CMD_MASK_COMMAND;
> -    if (command != NBD_CMD_DISC && (request.from + request.len) > exp->size) 
> {
> -            LOG("From: %" PRIu64 ", Len: %" PRIu32", Size: %" PRIu64
> -                ", Offset: %" PRIu64 "\n",
> -                request.from, request.len,
> -                (uint64_t)exp->size, (uint64_t)exp->dev_offset);
> -        LOG("requested operation past EOF--bad client?");
> -        goto invalid_request;
> -    }
> 
>      if (client->closing) {
>          /*
> @@ -1151,10 +1171,11 @@ static void nbd_trip(void *opaque)
>              goto out;
>          }
>          break;
> +
>      case NBD_CMD_DISC:
> -        TRACE("Request type is DISCONNECT");
> -        errno = 0;
> -        goto out;
> +        /* unreachable, thanks to special case in nbd_co_receive_request() */
> +        abort();
> +
>      case NBD_CMD_FLUSH:
>          TRACE("Request type is FLUSH");
> 
> @@ -1182,10 +1203,14 @@ static void nbd_trip(void *opaque)
>          break;
>      default:
>          LOG("invalid request type (%" PRIu32 ") received", request.type);
> -    invalid_request:
>          reply.error = EINVAL;
>      error_reply:
> -        if (nbd_co_send_reply(req, &reply, 0) < 0) {
> +        /* We must disconnect after replying with an error to
> +         * NBD_CMD_READ, since we choose not to send bogus filler
> +         * data; likewise after NBD_CMD_WRITE if we did not read the
> +         * payload. */
> +        if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ 
> ||
> +            (command == NBD_CMD_WRITE && !req->complete)) {

It's simpler to always set req->complete.  Putting everything together:

diff --git a/nbd/server.c b/nbd/server.c
index 4743316..73505dc 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1017,6 +1017,11 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
     TRACE("Decoding type");
 
     command = request->type & NBD_CMD_MASK_COMMAND;
+    if (command != NBD_CMD_WRITE) {
+        /* No payload, we are ready to read the next request.  */
+        req->complete = true;
+    }
+
     if (command == NBD_CMD_DISC) {
         /* Special case: we're going to disconnect without a reply,
          * whether or not flags, from, or len are bogus */
@@ -1064,7 +1069,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
         LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
             ", Size: %" PRIu64, request->from, request->len,
             (uint64_t)client->exp->size);
-        rc = -EINVAL;
+        rc = command == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
         goto out;
     }
 
@@ -1213,12 +1218,9 @@ static void nbd_trip(void *opaque)
         LOG("invalid request type (%" PRIu32 ") received", request.type);
         reply.error = EINVAL;
     error_reply:
-        /* We must disconnect after replying with an error to
-         * NBD_CMD_READ, since we choose not to send bogus filler
-         * data; likewise after NBD_CMD_WRITE if we did not read the
-         * payload. */
-        if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ ||
-            (command == NBD_CMD_WRITE && !req->complete)) {
+        /* We must disconnect after NBD_CMD_WRITE if we did not
+         * read the payload. */
+        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete)) {
             goto out;
         }
         break;

Paolo