[PATCH] thinkpad-acpi: Avoid heap buffer overrun

[PATCH] thinkpad-acpi: Avoid heap buffer overrun

[Michael Buesch]

Avoid a heap buffer overrun triggered by an integer overflow of the userspace
controlled "count" variable.
If userspace passes in a "count" of (size_t)-1l, the kmalloc size will overflow
to ((size_t)-1l + 2) = 1, so only one byte will be allocated. However, copy_from_user()
will attempt to copy 0xFFFFFFFF (or 0xFFFFFFFFFFFFFFFF on 64bit) bytes to the buffer.

A possible testcase could look like this:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>

int main(int argc, char **argv)
{
	int fd;
	char c;

	if (argc != 2) {
		printf("Usage: %s /proc/acpi/ibm/filename\n", argv[0]);
		return 1;
	}
	fd = open(argv[1], O_RDWR);
	if (fd < 0) {
		printf("Could not open proc file\n");
		return 1;
	}
	write(fd, &c, (size_t)-1l);
}

We avoid the integer overrun by putting an arbitrary limit on the count.
PAGE_SIZE sounds like a sane limit.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

---

This patch is completely untested due to lack of supported device.
The proc file is only writeable by root, so it's probably not exploitable as-is.

---
 drivers/platform/x86/thinkpad_acpi.c |    2 ++
 1 file changed, 2 insertions(+)

--- linux-2.6.orig/drivers/platform/x86/thinkpad_acpi.c
+++ linux-2.6/drivers/platform/x86/thinkpad_acpi.c
@@ -777,20 +777,22 @@ static int dispatch_procfs_read(char *pa
 static int dispatch_procfs_write(struct file *file,
 			const char __user *userbuf,
 			unsigned long count, void *data)
 {
 	struct ibm_struct *ibm = data;
 	char *kernbuf;
 	int ret;
 
 	if (!ibm || !ibm->write)
 		return -EINVAL;
+	if (count > PAGE_SIZE - 2)
+		return -EINVAL;
 
 	kernbuf = kmalloc(count + 2, GFP_KERNEL);
 	if (!kernbuf)
 		return -ENOMEM;
 
 	if (copy_from_user(kernbuf, userbuf, count)) {
 		kfree(kernbuf);
 		return -EFAULT;
 	}
 
 

-- 
Greetings, Michael.

Re: [PATCH] thinkpad-acpi: Avoid heap buffer overrun

[Michael Buesch]

On Tuesday 21 July 2009 12:16:17 Michael Buesch wrote:
> Avoid a heap buffer overrun triggered by an integer overflow of the userspace
> controlled "count" variable.
> If userspace passes in a "count" of (size_t)-1l, the kmalloc size will overflow
> to ((size_t)-1l + 2) = 1, so only one byte will be allocated. However, copy_from_user()
> will attempt to copy 0xFFFFFFFF (or 0xFFFFFFFFFFFFFFFF on 64bit) bytes to the buffer.
> 
> A possible testcase could look like this:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <fcntl.h>
> 
> int main(int argc, char **argv)
> {
> 	int fd;
> 	char c;
> 
> 	if (argc != 2) {
> 		printf("Usage: %s /proc/acpi/ibm/filename\n", argv[0]);
> 		return 1;
> 	}
> 	fd = open(argv[1], O_RDWR);
> 	if (fd < 0) {
> 		printf("Could not open proc file\n");
> 		return 1;
> 	}
> 	write(fd, &c, (size_t)-1l);
> }
> 
> We avoid the integer overrun by putting an arbitrary limit on the count.
> PAGE_SIZE sounds like a sane limit.
> 
> Signed-off-by: Michael Buesch <mb@bu3sch.de>

Whoops, forgot
Cc: stable@kernel.org


-- 
Greetings, Michael.

Re: [PATCH] thinkpad-acpi: Avoid heap buffer overrun

[Michael Buesch]

On Tuesday 21 July 2009 12:17:47 Michael Buesch wrote:
> On Tuesday 21 July 2009 12:16:17 Michael Buesch wrote:
> > Avoid a heap buffer overrun triggered by an integer overflow of the userspace
> > controlled "count" variable.
> > If userspace passes in a "count" of (size_t)-1l, the kmalloc size will overflow
> > to ((size_t)-1l + 2) = 1, so only one byte will be allocated. However, copy_from_user()
> > will attempt to copy 0xFFFFFFFF (or 0xFFFFFFFFFFFFFFFF on 64bit) bytes to the buffer.
> > 
> > A possible testcase could look like this:
> > 
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <unistd.h>
> > #include <fcntl.h>
> > 
> > int main(int argc, char **argv)
> > {
> > 	int fd;
> > 	char c;
> > 
> > 	if (argc != 2) {
> > 		printf("Usage: %s /proc/acpi/ibm/filename\n", argv[0]);
> > 		return 1;
> > 	}
> > 	fd = open(argv[1], O_RDWR);
> > 	if (fd < 0) {
> > 		printf("Could not open proc file\n");
> > 		return 1;
> > 	}
> > 	write(fd, &c, (size_t)-1l);
> > }

Hm, I just realized that for it to crash the source buffer must be big enough.
&c is on the stack. Maybe that's big enough to overrun the kmalloc()acted chunk
(there may be padding beyond the 1 byte). If it's not big enough, one must
malloc()ate a big buffer and pass it to write() instead.

This is all theoretical stuff, because I can't test it, but it looks sane. :)

> > We avoid the integer overrun by putting an arbitrary limit on the count.
> > PAGE_SIZE sounds like a sane limit.
> > 
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> 
> Whoops, forgot
> Cc: stable@kernel.org



-- 
Greetings, Michael.

Re: [PATCH] thinkpad-acpi: Avoid heap buffer overrun

[Henrique de Moraes Holschuh]

On Tue, 21 Jul 2009, Michael Buesch wrote:
> Avoid a heap buffer overrun triggered by an integer overflow of the userspace
> controlled "count" variable.
> If userspace passes in a "count" of (size_t)-1l, the kmalloc size will overflow
> to ((size_t)-1l + 2) = 1, so only one byte will be allocated. However, copy_from_user()
> will attempt to copy 0xFFFFFFFF (or 0xFFFFFFFFFFFFFFFF on 64bit) bytes to the buffer.

Good catch.  Thank you!

Patch acked, and queued.  I am sending it to Len Brown now (although I think
he is taking his post-rc2 time off, so it might take a week or so for it to
be sent to Linus).

It is a security hole alright, and while it is NOT open in a default
configuration, it CAN be used for local priviledge escalation on any system
where the distro or the local administratior used chown() or chmod() to
unrestrict access to /proc/acpi/ibm/*.

The code in question is in mainline since 2.6.12, and maybe even earlier.
Argh.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh