Re: [9fans] Kernel crash bug

[cinap_lenrek@gmx.de]

[-- 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