On 23 Apr 2016, at 00:40, Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
> ---
> 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
