Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932784AbdHWVRq (ORCPT ); Wed, 23 Aug 2017 17:17:46 -0400 Received: from mx1.gtisc.gatech.edu ([143.215.130.81]:58388 "EHLO mx1.gtisc.gatech.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932668AbdHWVRl (ORCPT ); Wed, 23 Aug 2017 17:17:41 -0400 X-Greylist: delayed 587 seconds by postgrey-1.27 at vger.kernel.org; Wed, 23 Aug 2017 17:17:41 EDT From: Meng Xu To: rjw@rjwysocki.net, lenb@kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org Cc: meng.xu@gatech.edu, sanidhya@gatech.edu, taesoo@gatech.edu, Meng Xu Subject: [PATCH] acpi: fix potential double-fetch bug Date: Wed, 23 Aug 2017 17:07:29 -0400 Message-Id: <1503522449-35440-1-git-send-email-meng.xu@gatech.edu> X-Mailer: git-send-email 2.7.4 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2422 Lines: 66 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 *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 --- 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