Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753867AbdIDPjP (ORCPT ); Mon, 4 Sep 2017 11:39:15 -0400 Received: from mail-yw0-f195.google.com ([209.85.161.195]:36059 "EHLO mail-yw0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753735AbdIDPjO (ORCPT ); Mon, 4 Sep 2017 11:39:14 -0400 X-Google-Smtp-Source: ADKCNb59fTsVgX3+lNer31G1KbuBVVWw3AnuloRFvVHBBZko3iFSjXOeQ/S3nIuU7cQsyf1iOnMO7Q== Subject: Re: [PATCH] nvdimm: fix potential double-fetch bug To: Dan Williams Cc: Meng Xu , "linux-nvdimm@lists.01.org" , "linux-kernel@vger.kernel.org" , sanidhya@gatech.edu, taesoo@gatech.edu, Jerry Hoemann References: <1503522466-35486-1-git-send-email-meng.xu@gatech.edu> From: Meng Xu Message-ID: <46e8e823-616e-c9f7-b2dc-0b39b1c75e71@gmail.com> Date: Mon, 4 Sep 2017 11:39:11 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3550 Lines: 83 Hi Dan, I have adjusted the patch as suggested by moving the check on nd_reserved2 to acpi_nfit_ctl(). The new patch can be found at https://marc.info/?l=linux-kernel&m=150453930712916&w=2 Best Regards, Meng On 08/31/2017 06:42 PM, Dan Williams wrote: > [ adding Jerry ] > > On Wed, Aug 23, 2017 at 2:07 PM, Meng Xu wrote: >> 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)); >> + } >> + > I think we're ok because the end point like acpi_nfit_ctl() is > responsible for re-validating the buffer. So what I would rather like > to see is deleting this loop: > > for (i = 0; i < ARRAY_SIZE(pkg.nd_reserved2); i++) > if (pkg.nd_reserved2[i]) > return -EINVAL; > > ...from __nd_ioctl() and move it into acpi_nfit_ctl() directly where it belongs.