Hi Rafael and Len, I have found a potential double-fetch operation (a TOCTOU case) in acpi/custom_method.c, which is explained in detail in the previous email. I'd like to seek your opinion about the patch I proposed and also your opinion about this problem in general.
The other two modules that have the same problem has been acknowledged and fixed: perf/core.c https://marc.info/?l=linux-kernel&m=150442907017508&w=2 drivers/nvdimm/bus.c https://marc.info/?l=linux-kernel&m=150454910315948&w=2 Best Regards, Meng On 08/23/2017 05:07 PM, Meng Xu wrote:
From: Meng Xu <mengxu.gat...@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 *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`drivers/acpi/custom_method.c (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.gat...@gmail.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;
