Re: [PATCH] nvdimm: fix potential double-fetch bug

From: Meng Xu
Date: Tue Sep 12 2017 - 18:50:02 EST

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,

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@xxxxxxxxxx> wrote:
From: Meng Xu <mengxu.gatech@xxxxxxxxx>

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

Signed-off-by: Meng Xu <mengxu.gatech@xxxxxxxxx>
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?