[9fans] Kernel crash bug

[9fans] Kernel crash bug

[Elizabeth Jones]

There exist crash bugs in some of the system call handlers to do with
string validation; sometimes, only the first byte of an argument string is
validated. The following program reliably causes a kernel panic for me:

#include <u.h>
#include <libc.h>

#define SEGBASE (char*)0x40000000
#define SEGSIZE 4096

int main() {
     segattach(0, "shared", SEGBASE, SEGSIZE);
     *(char*)(SEGBASE + SEGSIZE - 1) = 'a';
     exec((char*)SEGBASE + SEGSIZE - 1, nil);
     return 0;
}

-- Elly

Re: [9fans] Kernel crash bug

[cinap_lenrek]

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

maybe the kernel should use something like this to validate pointers
to null terminated strings?

(this assumes that validaddr for a byte will also be valid for the whole page)

void
validstraddr(char *p)
{
	char *x;

	for(;;){
		validaddr((ulong)p, 1, 0);
		x = (char*)(((ulong)p & ~(BY2PG-1))+BY2PG);
		for(; p < x; p++){
			if(*p == 0)
				return;
		}
	}
}

--
cinap

[-- Attachment #2: Type: message/rfc822, Size: 3019 bytes --]

From: Elizabeth Jones <elly1@andrew.cmu.edu>
To: 9fans@9fans.net
Subject: [9fans] Kernel crash bug
Date: Sat, 1 Aug 2009 17:01:26 -0400 (EDT)
Message-ID: <Pine.LNX.4.64-044.0908011700100.6538@unix10.andrew.cmu.edu>

There exist crash bugs in some of the system call handlers to do with
string validation; sometimes, only the first byte of an argument string is
validated. The following program reliably causes a kernel panic for me:

#include <u.h>
#include <libc.h>

#define SEGBASE (char*)0x40000000
#define SEGSIZE 4096

int main() {
     segattach(0, "shared", SEGBASE, SEGSIZE);
     *(char*)(SEGBASE + SEGSIZE - 1) = 'a';
     exec((char*)SEGBASE + SEGSIZE - 1, nil);
     return 0;
}

-- Elly

Re: [9fans] Kernel crash bug

[Charles Forsyth]

>maybe the kernel should use something like this to validate pointers
>to null terminated strings?

it could just call vmemchr correctly
or vmemchr could be a touch more careful

Re: [9fans] Kernel crash bug

[Russ Cox]

calling vmemchr assumes that the memory isn't being changed
by some other proc mapping the same page.  if you find the
NUL in one pass and then call strcpy or strlen on the pointer
later, the other proc might have pulled the NUL in the interim.

there is a function in the kernel called validnamedup
that both validates a string argument and at the same time
makes an in-kernel-memory copy.  it's the easiest safe
way to handle strings passed to the kernel.  namec uses
it and luckily almost every string pointer passed to the kernel
ends up being interpreted by namec.  exec is an exception.

when i was working on 9vx, i rewrote exec to remove
crashes like this one as well as a handful of other bugs.
the code is at
http://code.swtch.com/vx32/src/tip/src/9vx/a/sysproc.c#cl-220
and could easily be dropped back into plan 9.

russ

Re: [9fans] Kernel crash bug

[Elizabeth Jones]

On Sat, 1 Aug 2009, Russ Cox wrote:

> calling vmemchr assumes that the memory isn't being changed
> by some other proc mapping the same page.  if you find the
> NUL in one pass and then call strcpy or strlen on the pointer
> later, the other proc might have pulled the NUL in the interim.

With you so far.

> there is a function in the kernel called validnamedup
> that both validates a string argument and at the same time
> makes an in-kernel-memory copy.  it's the easiest safe
> way to handle strings passed to the kernel.  namec uses
> it and luckily almost every string pointer passed to the kernel
> ends up being interpreted by namec.  exec is an exception.

sysstat() uses namec in what I believe is considered to be the correct
fashion:

  959     validaddr(arg[0], 1, 0);
  960     c = namec((char*)arg[0], Aaccess, 0, 0);

namec() then calls validanamedup() which calls validname0(), which appears
to do the right thing. However, the following reliably crashes the kernel:

   1 #include <u.h>
   2 #include <libc.h>
   3
   4 #define SEGBASE (char*)0x40000000
   5 #define SEGSIZE 4096
   6
   7 int main() {
   8     uchar buf[128];
   9     segattach(0, "shared", SEGBASE, SEGSIZE);
  10     *(char*)(SEGBASE + SEGSIZE - 1) = 'a';
  11     stat((char*)SEGBASE + SEGSIZE - 1, buf, sizeof(buf));
  12     return 0;
  13 }

This suggests to me that something more unpleasant is afoot. Perhaps
validname0() is incorrect in some way?

> when i was working on 9vx, i rewrote exec to remove
> crashes like this one as well as a handful of other bugs.
> the code is at
> http://code.swtch.com/vx32/src/tip/src/9vx/a/sysproc.c#cl-220
> and could easily be dropped back into plan 9.
>
> russ

-- Elly

Re: [9fans] Kernel crash bug

[Russ Cox]

validname0 looks like it is trying to be too clever.
A better version of the first if statement would be:

	if((ulong)name < KZERO) {
		validaddr((ulong)name, 1, 0);
		if(!dup)
			print("warning: validname called from %lux with user pointer", pc);
		ename = vmemchr(name, 0, 1<<16);
	} else
		ename = memchr(name, 0, 1<<16);

Russ

Re: [9fans] Kernel crash bug

[cinap_lenrek]

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

yes:

	if((ulong)name < KZERO){
		validaddr((ulong)name, 1, 0);
		if(!dup)
			print("warning: validname called from %lux with user pointer", pc);
		p = name;
		t = BY2PG-((ulong)p&(BY2PG-1));
		while((ename=vmemchr(p, 0, t)) == nil){
->			p += t;
			t = BY2PG;
		}
	}else

when moving p to the start of the next page... it is not checked that this address is
valid as vmemchr() assumes the start address to be already checked, and it will
crash when vmemchr() touches it on the next round. vmemchr() will successive
check pages if the pointer to pointer + len span page boundries anyway so the
while is not really needed:

	name = aname;
	if((ulong)name < KZERO){
		validaddr((ulong)name, 1, 0);
		if(!dup)
			print("warning: validname called from %lux with user pointer", pc);
		ename = vmemchr(name, 0, (1<<16));
	}else

--
cinap

[-- Attachment #2: Type: message/rfc822, Size: 4601 bytes --]

From: Elizabeth Jones <elly1@andrew.cmu.edu>
To: Fans of the OS Plan 9 from Bell Labs <9fans@9fans.net>
Subject: Re: [9fans] Kernel crash bug
Date: Sat, 1 Aug 2009 18:47:48 -0400 (EDT)
Message-ID: <Pine.LNX.4.64-044.0908011839420.12216@unix10.andrew.cmu.edu>

On Sat, 1 Aug 2009, Russ Cox wrote:

> calling vmemchr assumes that the memory isn't being changed
> by some other proc mapping the same page.  if you find the
> NUL in one pass and then call strcpy or strlen on the pointer
> later, the other proc might have pulled the NUL in the interim.

With you so far.

> there is a function in the kernel called validnamedup
> that both validates a string argument and at the same time
> makes an in-kernel-memory copy.  it's the easiest safe
> way to handle strings passed to the kernel.  namec uses
> it and luckily almost every string pointer passed to the kernel
> ends up being interpreted by namec.  exec is an exception.

sysstat() uses namec in what I believe is considered to be the correct
fashion:

  959     validaddr(arg[0], 1, 0);
  960     c = namec((char*)arg[0], Aaccess, 0, 0);

namec() then calls validanamedup() which calls validname0(), which appears
to do the right thing. However, the following reliably crashes the kernel:

   1 #include <u.h>
   2 #include <libc.h>
   3
   4 #define SEGBASE (char*)0x40000000
   5 #define SEGSIZE 4096
   6
   7 int main() {
   8     uchar buf[128];
   9     segattach(0, "shared", SEGBASE, SEGSIZE);
  10     *(char*)(SEGBASE + SEGSIZE - 1) = 'a';
  11     stat((char*)SEGBASE + SEGSIZE - 1, buf, sizeof(buf));
  12     return 0;
  13 }

This suggests to me that something more unpleasant is afoot. Perhaps
validname0() is incorrect in some way?

> when i was working on 9vx, i rewrote exec to remove
> crashes like this one as well as a handful of other bugs.
> the code is at
> http://code.swtch.com/vx32/src/tip/src/9vx/a/sysproc.c#cl-220
> and could easily be dropped back into plan 9.
>
> russ

-- Elly

Re: [9fans] Kernel crash bug

[erik quanstrom]

diff -c /n/dump/2009/0801/sys/src/9/port/sysproc.c sysproc.c
/n/dump/2009/0801/sys/src/9/port/sysproc.c:234,247 - sysproc.c:234,248
  	ulong magic, text, entry, data, bss;
  	Tos *tos;

- 	validaddr(arg[0], 1, 0);
- 	file = (char*)arg[0];
+ 	file = nil;
  	indir = 0;
  	elem = nil;
  	if(waserror()){
  		free(elem);
+ 		free(file);
  		nexterror();
  	}
+ 	file = validnamedup((char*)arg[0], 1);
  	for(;;){
  		tc = namec(file, Aopen, OEXEC, 0);
  		if(waserror()){
diff -c /n/dump/2009/0801/sys/src/9/port/chan.c chan.c
/n/dump/2009/0801/sys/src/9/port/chan.c:1689,1701 - chan.c:1689,1698
  	if((ulong)name < KZERO){
  		validaddr((ulong)name, 1, 0);
  		if(!dup)
- 			print("warning: validname called from %lux with user pointer", pc);
+ 			print("warning: validname called from %#p with user pointer", pc);
  		p = name;
  		t = BY2PG-((ulong)p&(BY2PG-1));
- 		while((ename=vmemchr(p, 0, t)) == nil){
- 			p += t;
- 			t = BY2PG;
- 		}
+ 		ename = vmemchr(name, 0, 1<<16);
  	}else
  		ename = memchr(name, 0, (1<<16));

Re: [9fans] Kernel crash bug

[erik quanstrom]

On Sat Aug  1 21:40:18 EDT 2009, quanstro@quanstro.net wrote:
> diff -c /n/dump/2009/0801/sys/src/9/port/sysproc.c sysproc.c
> /n/dump/2009/0801/sys/src/9/port/sysproc.c:234,247 - sysproc.c:234,248

ready.  shoot.  aim.
sorry.  i sent the wrong patch.

i also should have mentioned that this patch is not as aggressive
about checking for arguments changing underfoot as russ'.  so
we can all anticipate the next program that'll be posted.

i do agree with charles that part of the solution is to ease fault386 to
only panic on addresses that obviously could have never been
valid, like 0, addresses in pci space, etc.

- erik

diffy -c sysproc.c chan.c
diff -c /n/dump/2009/0801/sys/src/9/port/sysproc.c sysproc.c
/n/dump/2009/0801/sys/src/9/port/sysproc.c:223,229 - sysproc.c:223,229
  	int i;
  	Chan *tc;
  	char **argv, **argp;
- 	char *a, *charp, *args, *file;
+ 	char *a, *charp, *args, *file, *file0;
  	char *progarg[sizeof(Exec)/2+1], *elem, progelem[64];
  	ulong ssize, spage, nargs, nbytes, n, bssend;
  	int indir;
/n/dump/2009/0801/sys/src/9/port/sysproc.c:234,247 - sysproc.c:234,248
  	ulong magic, text, entry, data, bss;
  	Tos *tos;

- 	validaddr(arg[0], 1, 0);
- 	file = (char*)arg[0];
+ 	file = nil;
  	indir = 0;
  	elem = nil;
  	if(waserror()){
  		free(elem);
+ 		free(file);
  		nexterror();
  	}
+ 	file = file0 = validnamedup((char*)arg[0], 1);
  	for(;;){
  		tc = namec(file, Aopen, OEXEC, 0);
  		if(waserror()){
/n/dump/2009/0801/sys/src/9/port/sysproc.c:375,380 - sysproc.c:376,382
  		charp += n;
  	}

+ 	free(file0);
  	free(up->text);
  	up->text = elem;
  	elem = nil;	/* so waserror() won't free elem */
diff -c /n/dump/2009/0801/sys/src/9/port/chan.c chan.c
/n/dump/2009/0801/sys/src/9/port/chan.c:1689,1701 - chan.c:1689,1698
  	if((ulong)name < KZERO){
  		validaddr((ulong)name, 1, 0);
  		if(!dup)
- 			print("warning: validname called from %lux with user pointer", pc);
+ 			print("warning: validname called from %#p with user pointer", pc);
  		p = name;
  		t = BY2PG-((ulong)p&(BY2PG-1));
- 		while((ename=vmemchr(p, 0, t)) == nil){
- 			p += t;
- 			t = BY2PG;
- 		}
+ 		ename = vmemchr(name, 0, 1<<16);
  	}else
  		ename = memchr(name, 0, (1<<16));

- erik