Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751622AbdITCfw (ORCPT ); Tue, 19 Sep 2017 22:35:52 -0400 Received: from mail-yw0-f193.google.com ([209.85.161.193]:36020 "EHLO mail-yw0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751595AbdITCfu (ORCPT ); Tue, 19 Sep 2017 22:35:50 -0400 X-Google-Smtp-Source: AOwi7QA71Tk6lcQJT6cv88zYNv0SYKpd/mJDUSVGkZKKXuKmLUOOS0/OaR16pzEHvSdPiBJIi29i3Q== Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: Re: [PATCH] nvdimm: fix potential double-fetch bug From: Meng Xu In-Reply-To: <898aae29-1ce5-9cc5-2e10-532eb72d50e8@gmail.com> Date: Tue, 19 Sep 2017 22:35:46 -0400 Cc: Meng Xu , "linux-nvdimm@lists.01.org" , "linux-kernel@vger.kernel.org" , sanidhya@gatech.edu, Taesoo Kim Message-Id: <6AB0FC4D-8D8D-4A9C-BD8D-2AF33B9F5A48@gmail.com> References: <1503522466-35486-1-git-send-email-meng.xu@gatech.edu> <20170912220322.GA19642@anatevka.americas.hpqcorp.net> <898aae29-1ce5-9cc5-2e10-532eb72d50e8@gmail.com> To: Jerry.Hoemann@hpe.com, Dan Williams X-Mailer: Apple Mail (2.3273) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v8K2Zv7K012477 Content-Length: 6286 Lines: 167 Hi Jerry and Dan, Sorry for the late reply. I looked at this issue again and found that simple patches like memcmp(buf, in_env, in_len) && memcmp(buf + in_len, out_env, out_len) will only work in the case of (cmd == ND_CMD_CALL) and does not apply to other cmd. In fact, I fail to find a patch that is small enough (i.e., within 20 lines of modifications) to fix this problem. This is largely because there are too many factors that can affect the in_size and out_size (i.e., the if-else branches in nd_cmd_in_size() and nd_cmd_out_size()). One option maybe is to split the loops for processing input and output envelopes early, like this: if (nvdimm && cmd == ND_CMD_SET_CONFIG_DATA) { /* process an input envelope */ for (i = 0; i < desc->in_num; i++) {} …… /* process an output envelope */ for (i = 0; i < desc->out_num; i++) {} } else if (nvdimm && cmd == ND_CMD_VENDOR { /* process an input envelope */ for (i = 0; i < desc->in_num; i++) {} …… /* process an output envelope */ for (i = 0; i < desc->out_num; i++) {} } else if (cmd == ND_CMD_CALL) { /* process an input envelope */ for (i = 0; i < desc->in_num; i++) {} …… /* process an output envelope */ for (i = 0; i < desc->out_num; i++) {} } But I guess this will require some major refactoring of the code, which I am not sure is a good idea or is in my capability. Please let me know your thoughts on this matter. Thanks. Best Regards, Meng > On Sep 12, 2017, at 6:49 PM, Meng Xu wrote: > > Hi Jerry, > > Thank you for the question. Yes, these double copies > do seem to present an issue. > > __nd_ioctl() and acpi_nfit_ctl() both use the same way > to derive `out_size`, but based on different data fetches. > > A simple patch would be > memcmp(buf, in_env, in_len) > memcmp(buf + in_len, out_env, out_len) > > I am not sure I captured all the subtle issues with such a > patch so please allow me some time to create and test it. > > Best regards, > Meng > > On 09/12/2017 06:03 PM, Jerry Hoemann wrote: >> On Thu, Aug 31, 2017 at 03:42:52PM -0700, 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. >> Sorry for the delay, I've been away. >> >> I'm okay with moving the test to the beginning of acpi_nfit_ctl. If/When the reserved >> fields are defined/used, we may need to tweak that. But we can cross that >> bridge when it comes. >> >> However, I do have a question. >> >> There are two for loops in __nd_ioctl that process desc->in_num and desc->out_num >> respectively. These loops also copy_from_user before >> >> buf = vmalloc(buf_len); >> if (!buf) >> return -ENOMEM; >> >> if (copy_from_user(buf, p, buf_len)) { >> rc = -EFAULT; >> goto out; >> } >> >> >> Do these double copy instances present any problems? >> >> >