From patchwork Thu Jun 16 14:16:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 9181295 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E139C60776 for ; Thu, 16 Jun 2016 15:43:53 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D1A7A27248 for ; Thu, 16 Jun 2016 15:43:53 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C65C028364; Thu, 16 Jun 2016 15:43:53 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 3748827248 for ; Thu, 16 Jun 2016 15:43:53 +0000 (UTC) Received: from localhost ([::1]:50211 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDZSR-0002p9-OR for patchwork-qemu-devel@patchwork.kernel.org; Thu, 16 Jun 2016 11:43:51 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48599) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDY6Q-00074D-Kb for qemu-devel@nongnu.org; Thu, 16 Jun 2016 10:17:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDY6O-000370-0V for qemu-devel@nongnu.org; Thu, 16 Jun 2016 10:17:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61949) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDY6N-00036w-Nn for qemu-devel@nongnu.org; Thu, 16 Jun 2016 10:16:59 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 61E83C04B304 for ; Thu, 16 Jun 2016 14:16:59 +0000 (UTC) Received: from donizetti.redhat.com (ovpn-112-58.ams2.redhat.com [10.36.112.58]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5GEGQgt016497; Thu, 16 Jun 2016 10:16:58 -0400 From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Thu, 16 Jun 2016 16:16:15 +0200 Message-Id: <1466086585-16526-21-git-send-email-pbonzini@redhat.com> In-Reply-To: <1466086585-16526-1-git-send-email-pbonzini@redhat.com> References: <1466086585-16526-1-git-send-email-pbonzini@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 16 Jun 2016 14:16:59 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL 20/30] nbd: Improve server handling of bogus commands X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP From: Eric Blake 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 where from + len overflows, 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. 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 Message-Id: <1463006384-7734-5-git-send-email-eblake@redhat.com> Signed-off-by: Paolo Bonzini --- nbd/server.c | 66 +++++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 19 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 8b0fc0f..0e71f78 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,31 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque goto out; } + 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 */ + 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"); + 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_READ || command == NBD_CMD_WRITE) { if (request->len > NBD_MAX_BUFFER_SIZE) { LOG("len (%" PRIu32" ) is larger than max len (%u)", @@ -1039,7 +1061,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 = command == NBD_CMD_WRITE ? -ENOSPC : -EINVAL; + goto out; + } + rc = 0; out: @@ -1082,14 +1115,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 +1176,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"); @@ -1190,10 +1216,12 @@ 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 NBD_CMD_WRITE if we did not + * read the payload. + */ + if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete) { goto out; } break;