Re: [Qemu-devel] [PATCH v3 03/44] nbd: Improve server handling of bogus commands

[Alex Bligh]

On 23 Apr 2016, at 00:40, Eric Blake <address@hidden> 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.
> 
> 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 0a003e4..6a6b5a2 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 {
> @@ -985,7 +986,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;
> @@ -1003,16 +1010,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");

I realise this is unchanged since the previous code, but why
'you're probably being attacked'? More likely you're probably
using a buggy client.

> +        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)",
> @@ -1035,7 +1052,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;
> +        goto out;
> +    }
> +
>     rc = 0;
> 
> out:
> @@ -1077,14 +1105,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)) {
>             goto out;
>         }
>         break;
> -- 
> 2.5.5
> 
> 

-- 
Alex Bligh