* [9fans] Kernel crash bug @ 2009-08-01 21:01 Elizabeth Jones 2009-08-01 21:37 ` cinap_lenrek 0 siblings, 1 reply; 9+ messages in thread From: Elizabeth Jones @ 2009-08-01 21:01 UTC (permalink / raw) To: 9fans 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] Kernel crash bug 2009-08-01 21:01 [9fans] Kernel crash bug Elizabeth Jones @ 2009-08-01 21:37 ` cinap_lenrek 2009-08-01 22:01 ` Charles Forsyth 0 siblings, 1 reply; 9+ messages in thread From: cinap_lenrek @ 2009-08-01 21:37 UTC (permalink / raw) To: 9fans [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] Kernel crash bug 2009-08-01 21:37 ` cinap_lenrek @ 2009-08-01 22:01 ` Charles Forsyth 2009-08-01 22:08 ` Russ Cox 0 siblings, 1 reply; 9+ messages in thread From: Charles Forsyth @ 2009-08-01 22:01 UTC (permalink / raw) To: 9fans >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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] Kernel crash bug 2009-08-01 22:01 ` Charles Forsyth @ 2009-08-01 22:08 ` Russ Cox 2009-08-01 22:47 ` Elizabeth Jones 0 siblings, 1 reply; 9+ messages in thread From: Russ Cox @ 2009-08-01 22:08 UTC (permalink / raw) To: Fans of the OS Plan 9 from Bell Labs 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] Kernel crash bug 2009-08-01 22:08 ` Russ Cox @ 2009-08-01 22:47 ` Elizabeth Jones 2009-08-01 23:09 ` Russ Cox ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Elizabeth Jones @ 2009-08-01 22:47 UTC (permalink / raw) To: Fans of the OS Plan 9 from Bell Labs 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] Kernel crash bug 2009-08-01 22:47 ` Elizabeth Jones @ 2009-08-01 23:09 ` Russ Cox 2009-08-01 23:15 ` cinap_lenrek 2009-08-02 1:38 ` erik quanstrom 2 siblings, 0 replies; 9+ messages in thread From: Russ Cox @ 2009-08-01 23:09 UTC (permalink / raw) To: Fans of the OS Plan 9 from Bell Labs 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] Kernel crash bug 2009-08-01 22:47 ` Elizabeth Jones 2009-08-01 23:09 ` Russ Cox @ 2009-08-01 23:15 ` cinap_lenrek 2009-08-02 1:38 ` erik quanstrom 2 siblings, 0 replies; 9+ messages in thread From: cinap_lenrek @ 2009-08-01 23:15 UTC (permalink / raw) To: 9fans [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] Kernel crash bug 2009-08-01 22:47 ` Elizabeth Jones 2009-08-01 23:09 ` Russ Cox 2009-08-01 23:15 ` cinap_lenrek @ 2009-08-02 1:38 ` erik quanstrom 2009-08-02 2:13 ` erik quanstrom 2 siblings, 1 reply; 9+ messages in thread From: erik quanstrom @ 2009-08-02 1:38 UTC (permalink / raw) To: 9fans 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)); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] Kernel crash bug 2009-08-02 1:38 ` erik quanstrom @ 2009-08-02 2:13 ` erik quanstrom 0 siblings, 0 replies; 9+ messages in thread From: erik quanstrom @ 2009-08-02 2:13 UTC (permalink / raw) To: 9fans 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-08-02 2:13 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-08-01 21:01 [9fans] Kernel crash bug Elizabeth Jones 2009-08-01 21:37 ` cinap_lenrek 2009-08-01 22:01 ` Charles Forsyth 2009-08-01 22:08 ` Russ Cox 2009-08-01 22:47 ` Elizabeth Jones 2009-08-01 23:09 ` Russ Cox 2009-08-01 23:15 ` cinap_lenrek 2009-08-02 1:38 ` erik quanstrom 2009-08-02 2:13 ` erik quanstrom
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).