From patchwork Wed Aug 23 21:07:46 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Meng Xu X-Patchwork-Id: 9918379 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 EEB65603FA for ; Wed, 23 Aug 2017 21:08:29 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D7A3A28A5C for ; Wed, 23 Aug 2017 21:08:29 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CB6B328A66; Wed, 23 Aug 2017 21:08:29 +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=-1.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from ml01.01.org (ml01.01.org [198.145.21.10]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id EB3DA28A71 for ; Wed, 23 Aug 2017 21:08:28 +0000 (UTC) Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id 3A3E62095E007; Wed, 23 Aug 2017 14:05:25 -0700 (PDT) X-Original-To: linux-nvdimm@lists.01.org Delivered-To: linux-nvdimm@lists.01.org Received: from mx1.gtisc.gatech.edu (mx1.gtisc.gatech.edu [143.215.130.81]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 82E2D21E47D65 for ; Wed, 23 Aug 2017 14:05:23 -0700 (PDT) Received: from bombshell.gtisc.gatech.edu (unknown [172.30.240.76]) by mx1.gtisc.gatech.edu (Postfix) with SMTP id 8F2F1C2207; Wed, 23 Aug 2017 17:07:51 -0400 (EDT) Received: (nullmailer pid 35529 invoked by uid 1026); Wed, 23 Aug 2017 21:07:50 -0000 From: Meng Xu To: dan.j.williams@intel.com, linux-nvdimm@lists.01.org, linux-kernel@vger.kernel.org Subject: [PATCH] nvdimm: fix potential double-fetch bug Date: Wed, 23 Aug 2017 17:07:46 -0400 Message-Id: <1503522466-35486-1-git-send-email-meng.xu@gatech.edu> X-Mailer: git-send-email 2.7.4 X-BeenThere: linux-nvdimm@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "Linux-nvdimm developer list." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: taesoo@gatech.edu, meng.xu@gatech.edu, Meng Xu , sanidhya@gatech.edu MIME-Version: 1.0 Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" X-Virus-Scanned: ClamAV using ClamSMTP From: Meng Xu While examining the kernel source code, I found a dangerous operation that could turn into a double-fetch situation (a race condition bug) where the same userspace memory region are fetched twice into kernel with sanity checks after the first fetch while missing checks after the second fetch. In the case of _IOC_NR(ioctl_cmd) == ND_CMD_CALL: 1. The first fetch happens in line 935 copy_from_user(&pkg, p, sizeof(pkg) 2. subsequently `pkg.nd_reserved2` is asserted to be all zeroes (line 984 to 986). 3. The second fetch happens in line 1022 copy_from_user(buf, p, buf_len) 4. Given that `p` can be fully controlled in userspace, an attacker can race condition to override the header part of `p`, say, `((struct nd_cmd_pkg *)p)->nd_reserved2` to arbitrary value (say nine 0xFFFFFFFF for `nd_reserved2`) after the first fetch but before the second fetch. The changed value will be copied to `buf`. 5. There is no checks on the second fetches until the use of it in line 1034: nd_cmd_clear_to_send(nvdimm_bus, nvdimm, cmd, buf) and line 1038: nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len, &cmd_rc) which means that the assumed relation, `p->nd_reserved2` are all zeroes might not hold after the second fetch. And once the control goes to these functions we lose the context to assert the assumed relation. 6. Based on my manual analysis, `p->nd_reserved2` is not used in function `nd_cmd_clear_to_send` and potential implementations of `nd_desc->ndctl` so there is no working exploit against it right now. However, this could easily turns to an exploitable one if careless developers start to use `p->nd_reserved2` later and assume that they are all zeroes. Proposed patch: The patch explicitly overrides `buf->nd_reserved2` after the second fetch with the value `pkg.nd_reserved2` from the first fetch. In this way, it is assured that the relation, `buf->nd_reserved2` are all zeroes, holds after the second fetch. Signed-off-by: Meng Xu --- drivers/nvdimm/bus.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index 937fafa..20c4d0f 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -1024,6 +1024,12 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, goto out; } + if (cmd == ND_CMD_CALL) { + struct nd_cmd_pkg *hdr = (struct nd_cmd_pkg *)buf; + memcpy(hdr->nd_reserved2, pkg.nd_reserved2, + sizeof(pkg.nd_reserved2)); + } + nvdimm_bus_lock(&nvdimm_bus->dev); rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf); if (rc)