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