* [PATCH] nvdimm: fix potential double-fetch bug @ 2017-08-23 21:07 Meng Xu 2017-08-31 22:42 ` Dan Williams 0 siblings, 1 reply; 6+ messages in thread From: Meng Xu @ 2017-08-23 21:07 UTC (permalink / raw) To: dan.j.williams, linux-nvdimm, linux-kernel Cc: taesoo, meng.xu, Meng Xu, sanidhya From: Meng Xu <mengxu.gatech@gmail.com> 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 <mengxu.gatech@gmail.com> --- 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) -- 2.7.4 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] nvdimm: fix potential double-fetch bug 2017-08-23 21:07 [PATCH] nvdimm: fix potential double-fetch bug Meng Xu @ 2017-08-31 22:42 ` Dan Williams 2017-09-04 15:39 ` Meng Xu 2017-09-12 22:03 ` Jerry Hoemann 0 siblings, 2 replies; 6+ messages in thread From: Dan Williams @ 2017-08-31 22:42 UTC (permalink / raw) To: Meng Xu Cc: taesoo, linux-nvdimm@lists.01.org, sanidhya, linux-kernel@vger.kernel.org, Meng Xu [ adding Jerry ] On Wed, Aug 23, 2017 at 2:07 PM, Meng Xu <meng.xu@gatech.edu> wrote: > From: Meng Xu <mengxu.gatech@gmail.com> > > 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 <mengxu.gatech@gmail.com> > --- > 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. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvdimm: fix potential double-fetch bug 2017-08-31 22:42 ` Dan Williams @ 2017-09-04 15:39 ` Meng Xu 2017-09-12 22:03 ` Jerry Hoemann 1 sibling, 0 replies; 6+ messages in thread From: Meng Xu @ 2017-09-04 15:39 UTC (permalink / raw) To: Dan Williams Cc: taesoo, linux-nvdimm@lists.01.org, sanidhya, linux-kernel@vger.kernel.org, Meng Xu 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 <meng.xu@gatech.edu> wrote: >> From: Meng Xu <mengxu.gatech@gmail.com> >> >> 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 <mengxu.gatech@gmail.com> >> --- >> 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. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvdimm: fix potential double-fetch bug 2017-08-31 22:42 ` Dan Williams 2017-09-04 15:39 ` Meng Xu @ 2017-09-12 22:03 ` Jerry Hoemann 2017-09-12 22:49 ` Meng Xu 1 sibling, 1 reply; 6+ messages in thread From: Jerry Hoemann @ 2017-09-12 22:03 UTC (permalink / raw) To: Dan Williams Cc: taesoo, linux-nvdimm@lists.01.org, sanidhya, Meng Xu, linux-kernel@vger.kernel.org, Meng Xu 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 <meng.xu@gatech.edu> wrote: > > From: Meng Xu <mengxu.gatech@gmail.com> > > > > 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 <mengxu.gatech@gmail.com> > > --- > > 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? -- ----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise ----------------------------------------------------------------------------- _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvdimm: fix potential double-fetch bug 2017-09-12 22:03 ` Jerry Hoemann @ 2017-09-12 22:49 ` Meng Xu 2017-09-20 2:35 ` Meng Xu 0 siblings, 1 reply; 6+ messages in thread From: Meng Xu @ 2017-09-12 22:49 UTC (permalink / raw) To: Jerry.Hoemann, Dan Williams Cc: taesoo, Meng Xu, linux-kernel@vger.kernel.org, sanidhya, linux-nvdimm@lists.01.org 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 <meng.xu@gatech.edu> wrote: >>> From: Meng Xu <mengxu.gatech@gmail.com> >>> >>> 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 <mengxu.gatech@gmail.com> >>> --- >>> 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? > > _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvdimm: fix potential double-fetch bug 2017-09-12 22:49 ` Meng Xu @ 2017-09-20 2:35 ` Meng Xu 0 siblings, 0 replies; 6+ messages in thread From: Meng Xu @ 2017-09-20 2:35 UTC (permalink / raw) To: Jerry.Hoemann, Dan Williams Cc: Taesoo Kim, Meng Xu, linux-kernel@vger.kernel.org, sanidhya, linux-nvdimm@lists.01.org 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 <mengxu.gatech@gmail.com> 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 <meng.xu@gatech.edu> wrote: >>>> From: Meng Xu <mengxu.gatech@gmail.com> >>>> >>>> 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 <mengxu.gatech@gmail.com> >>>> --- >>>> 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? >> >> > _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-20 2:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-23 21:07 [PATCH] nvdimm: fix potential double-fetch bug Meng Xu 2017-08-31 22:42 ` Dan Williams 2017-09-04 15:39 ` Meng Xu 2017-09-12 22:03 ` Jerry Hoemann 2017-09-12 22:49 ` Meng Xu 2017-09-20 2:35 ` Meng Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox