linux-kernel - [PATCH] acpi: fix potential double-fetch bug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <1503522449-35440-1-git-send-email-meng.xu@gatech.edu>
Date:   Wed, 23 Aug 2017 17:07:29 -0400
From:   Meng Xu <meng.xu@...ech.edu>
To:     rjw@...ysocki.net, lenb@...nel.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     meng.xu@...ech.edu, sanidhya@...ech.edu, taesoo@...ech.edu,
        Meng Xu <mengxu.gatech@...il.com>
Subject: [PATCH] acpi: fix potential double-fetch bug

From: Meng Xu <mengxu.gatech@...il.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 *ppos == 0:

1. The first fetch happens in line 36
copy_from_user(&table, user_buf, sizeof(struct acpi_table_header)))

2. Subsequently `table.length` variable is used to allocate the `buf`
(line 40).

3. The second fetch happens in line 54
copy_from_user(buf + (*ppos), user_buf, count)

4. Given that `user_buf` can be fully controlled in userspace, an attacker can
race condition to override the header part of `user_buf`, say,
`((struct acpi_table_header *)user_buf)->length` to arbitrary value
(say 0xFFFFFFFF) after the first fetch but before the second fetch. The changed
value will be copied to `buf`.

5. There is no checks on `((struct acpi_table_header *)buf)->length` until the
use of it in line 64 status = acpi_install_method(buf), which means that the
assumed relation, `buf->length` == length of the buffer, might not hold after
the second fetch. And once the control goes to function `acpi_install_method`,
we lose the context to assert that.

6. It is lucky that `buf->length` is not used in function `acpi_install_method`
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
`buf->length` later.

Proposed patch:

The patch explicitly overrides `buf->length` after the second fetch with the
value `max_size` from the first fetch. In this way, it is assured that the
relation, `buf->length` == length of the buffer, holds after the second fetch.

Signed-off-by: Meng Xu <mengxu.gatech@...il.com>
---
 drivers/acpi/custom_method.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index c68e724..eea7986 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -57,6 +57,13 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
 		return -EFAULT;
 	}

+	if (!(*ppos)) {
+		struct acpi_table_header *hdr =
+			ACPI_CAST_PTR(struct acpi_table_header, buf);
+
+		hdr->length = max_size;
+	}
+
 	uncopied_bytes -= count;
 	*ppos += count;

-- 
2.7.4