From patchwork Mon Jun 13 12:19:46 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 9172793 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 72E51604DB for ; Mon, 13 Jun 2016 12:20:39 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6311B25404 for ; Mon, 13 Jun 2016 12:20:39 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 57B5C27C0C; Mon, 13 Jun 2016 12:20:39 +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.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID 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 AA1C725404 for ; Mon, 13 Jun 2016 12:20:38 +0000 (UTC) Received: from localhost ([::1]:56102 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCQr6-0000zH-SS for patchwork-qemu-devel@patchwork.kernel.org; Mon, 13 Jun 2016 08:20:36 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33852) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCQqU-0000uw-8S for qemu-devel@nongnu.org; Mon, 13 Jun 2016 08:20:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCQqR-0005Hf-KI for qemu-devel@nongnu.org; Mon, 13 Jun 2016 08:19:57 -0400 Received: from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241]:33548) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCQqL-0005FT-Bl; Mon, 13 Jun 2016 08:19:49 -0400 Received: by mail-wm0-x241.google.com with SMTP id r5so14448449wmr.0; Mon, 13 Jun 2016 05:19:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=YxNaNk6Kpa4mpggcnEg9MDLZgO+1xyvSEXmtzQbaA58=; b=z4XMJLKOx/BLcgWU7tXSkLC4831Ab6a+2K7zqED4SRfqTaN5k6OlcuJpPz8doW7Mih xFrdNHahDtB39UGMHRAu0yAmgp6oYZupdK+STmizdf22bOe+BqcEFbBEzUZ0tgykki8v BcTBhdUd/KI0ynpDLSSYQ0mqIDgFTfZhc4W2xdpgvOCvWe8moJiYS33fXpbV2Zk1fsAA SBqdN25eVAZDfbdwKj9MgtBnObkpPGbswOsLq11Y2xDH3f9Nc1moF9lqHKhfEF4xiFBy 4uQy/G9H6PXWyyb5XfsJ+kvp7x9Roc3kVmKrjQ/gncDC1FirJb90VO7KvMLS99DMIZV9 2TLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:subject:to:references:cc:from:message-id :date:user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=YxNaNk6Kpa4mpggcnEg9MDLZgO+1xyvSEXmtzQbaA58=; b=SiQQ+YzhvCCrR+RfypIznypWFb/YDf7SOINmbXGEPX9nzCwEN3qprC+dirN70KGn10 SK4SQccS395Sxm89ZS0RjSmxid2d4TnuB0MDXaHuqMijX+Z6CXTYOkatBBAbqr+4zsQQ PPKY7K6RhFBGrPDgo+1x7QIhzgMrlQi6GxFgInCcfBrtZVNudhY27drLumWJbbN63ObI V36FAiR3rL99fIoAqiLaQ+TvdhTIHn+oosTWvy5pQhQJXoF2jx0LL7RPvDOFY95lfzfz 5PY2ERBhnIZO2BM9qIzQk3rrW2+WITMeZiXHxxOYOMhox1VMskMIcyAIx/Ee1MXHIs3+ Nw8A== X-Gm-Message-State: ALyK8tJbesEwK+341YYUJT7NrwRI2UHzL7ne96hjQiOYfadeWcS3fqYsiMZ+WLpKhm8qHQ== X-Received: by 10.194.221.37 with SMTP id qb5mr802324wjc.171.1465820388398; Mon, 13 Jun 2016 05:19:48 -0700 (PDT) Received: from [192.168.10.165] (94-39-188-118.adsl-ull.clienti.tiscali.it. [94.39.188.118]) by smtp.googlemail.com with ESMTPSA id z1sm23905572wju.32.2016.06.13.05.19.47 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 13 Jun 2016 05:19:47 -0700 (PDT) To: Eric Blake , qemu-devel@nongnu.org References: <1463006384-7734-1-git-send-email-eblake@redhat.com> <1463006384-7734-5-git-send-email-eblake@redhat.com> From: Paolo Bonzini Message-ID: <7c7619a2-a6f3-c30c-c8d0-aac1f96ae661@redhat.com> Date: Mon, 13 Jun 2016 14:19:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <1463006384-7734-5-git-send-email-eblake@redhat.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a00:1450:400c:c09::241 Subject: Re: [Qemu-devel] [PATCH v4 04/11] 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: , Cc: alex@alex.org.uk, qemu-block@nongnu.org Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 > --- > 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: Paolo 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;