Message ID | 1503522466-35486-1-git-send-email-meng.xu@gatech.edu (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
[ 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.
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.
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?
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? > >
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? >> >> >
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)