From owner-freebsd-arch Fri May 31 13:40:29 2002 Delivered-To: freebsd-arch@freebsd.org Received: from sccrmhc03.attbi.com (sccrmhc03.attbi.com [204.127.202.63]) by hub.freebsd.org (Postfix) with ESMTP id 62F5C37B403 for ; Fri, 31 May 2002 13:40:12 -0700 (PDT) Received: from InterJet.elischer.org ([12.232.206.8]) by sccrmhc03.attbi.com (InterMail vM.4.01.03.27 201-229-121-127-20010626) with ESMTP id <20020531204011.MEEN20219.sccrmhc03.attbi.com@InterJet.elischer.org>; Fri, 31 May 2002 20:40:11 +0000 Received: from localhost (localhost.elischer.org [127.0.0.1]) by InterJet.elischer.org (8.9.1a/8.9.1) with ESMTP id NAA29828; Fri, 31 May 2002 13:36:16 -0700 (PDT) Date: Fri, 31 May 2002 13:36:15 -0700 (PDT) From: Julian Elischer To: Archie Cobbs Cc: Jake Burkholder , freebsd-arch@FreeBSD.ORG Subject: Re: Kernel stack overflow detection? In-Reply-To: <200205291920.g4TJKkE92786@arch20m.dellroad.org> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Wed, 29 May 2002, Archie Cobbs wrote: > Jake Burkholder writes: > > > If INVARIANTS doesn't do so already, I'd like to propose to write > > > up an INVARIANTS check that would validate that the kernel stack > > > has not overflowed. However I'm curious if anyone has done this > > > already and/or what the right way to go about it would be. E.g, add > > > an extra stack page with read-only protection? Any hints appreciated. > > > > -current has a guard page, -stable does not. Also, in -current the > > u. area and the pcb were moved so the kernel stack grows away from > > them, instead of towards. Either of these changes should be relatively > > easy to back port. In -current the U-area and the stack are becoming completely separate, as the stack and pcb is a per-thread thing and the remaining parts of the u-area (signal stuff) is a per-process thing. > > > > Note that on x86 a page fault due a stack overflow will cause a double > > fault; the double fault handler uses a task gate which does a hardware > > context switch to get off of the bad stack. > > So here's a first attempt at a patch for -stable. This patch does > not try to be too ambitious: > > - It doesn't move the relative position of the stack in the u. area > - It only works for i386 > > This is a MFC of the -current option KSTACK_GUARD although it's > done differently (-current and -stable are different anyway in > this area). > > It appears that the -current KSTACK_GUARD doesn't mark the page > as read-only (?), whereas this patch does. The advantage is automated > hardware detection of stack overflow, but the disadvantage is > that we have to increase UPAGES by 2 instead of 1 to avoid making > the writable portion of the stack smaller (because before it was > not a multiple of PAGE_SIZE). the guard page should be completely un-mapped. > > Eventually, this option can be applied to other architectures and > moved from conf/options.i386 into conf/options. > > Thanks for any reviews/comments. > > -Archie > > __________________________________________________________________________ > Archie Cobbs * Packet Design * http://www.packetdesign.com > > --- i386/i386/locore.s Thu Sep 20 02:29:23 2001 > +++ i386/i386/locore.s Wed May 29 11:30:20 2002 > @@ -813,9 +813,34 @@ > fillkptphys($PG_RW) > > /* Map proc0's UPAGES in the physical way ... */ > +#ifdef KSTACK_GUARD > + /* > + * Map the 1st and the 3rd UPAGES pages as writable and the 2nd > + * page as read-only to detect kernel stack overflows. > + * > + * Because of the way fillkptphys() works we have to do this in > + * three stages: 1st page RW, 2nd page RO, and pages 3-N RW. > + */ > + movl R(p0upa), %eax > + movl $1, %ecx > + fillkptphys($PG_RW) This was the u-area.. ok. though we probably need to check sizeof(strucr user). > + > + movl R(p0upa), %eax > + addl $PAGE_SIZE, %eax > + movl $1, %ecx > + fillkptphys($PG_V) > + This was the guard page.. If we don't call fillkptphys, but just skip over it I think we'd be ok.. I haven't looked at fillkptphys() to see if we need to increment something, probably not. > + movl R(p0upa), %eax > + addl $PAGE_SIZE, %eax > + addl $PAGE_SIZE, %eax > + movl $UPAGES, %ecx > + subl $2, %ecx no, we should split upages into KSTACK_PAGES and USER_PAGES as we did in -current > + fillkptphys($PG_RW) > +#else > movl R(p0upa), %eax > movl $UPAGES, %ecx > fillkptphys($PG_RW) > +#endif > > /* Map ISA hole */ > movl $ISA_HOLE_START, %eax > --- i386/i386/pmap.c Thu Jan 3 17:17:33 2002 > +++ i386/i386/pmap.c Wed May 29 11:45:39 2002 > @@ -890,7 +890,17 @@ > /* > * Enter the page into the kernel address space. > */ > +#ifdef KSTACK_GUARD > + /* Make bottom page of stack area un-writable */ if you just left it alone it would already be so.. > + if (i == 1) > + *(ptek + i) = VM_PAGE_TO_PHYS(m) | PG_V | pgeflag; > + else { > + *(ptek + i) = VM_PAGE_TO_PHYS(m) > + | PG_RW | PG_V | pgeflag; > + } > +#else > *(ptek + i) = VM_PAGE_TO_PHYS(m) | PG_RW | PG_V | pgeflag; > +#endif > if (oldpte) { > if ((oldpte & PG_G) || (cpu_class > CPUCLASS_386)) { > invlpg((vm_offset_t) up + i * PAGE_SIZE); > @@ -901,7 +911,15 @@ > > vm_page_wakeup(m); > vm_page_flag_clear(m, PG_ZERO); > +#ifdef KSTACK_GUARD > + /* Make bottom page of stack area un-writable */ > + if (i == 1) > + vm_page_flag_set(m, PG_MAPPED); > + else > + vm_page_flag_set(m, PG_MAPPED | PG_WRITEABLE); > +#else > vm_page_flag_set(m, PG_MAPPED | PG_WRITEABLE); > +#endif > m->valid = VM_PAGE_BITS_ALL; > } > if (updateneeded) > @@ -997,7 +1015,15 @@ > > vm_page_wire(m); > vm_page_wakeup(m); > +#ifdef KSTACK_GUARD > + /* Make bottom page of stack area un-writable */ > + if (i == 1) > + vm_page_flag_set(m, PG_MAPPED); > + else > + vm_page_flag_set(m, PG_MAPPED | PG_WRITEABLE); > +#else > vm_page_flag_set(m, PG_MAPPED | PG_WRITEABLE); > +#endif > } > } > > --- i386/include/param.h Mon Sep 24 23:14:07 2001 > +++ i386/include/param.h Wed May 29 11:34:56 2002 > @@ -110,7 +110,11 @@ > #define MAXDUMPPGS (DFLTPHYS/PAGE_SIZE) > > #define IOPAGES 2 /* pages of i/o permission bitmap */ > +#ifdef KSTACK_GUARD > +#define UPAGES 5 /* pages of u-area + kernel stack guard */ > +#else > #define UPAGES 3 /* pages of u-area */ > +#endif the patches to separate out the u-area and the stack were pretty trivial check the code in -current. > > /* > * Ceiling on amount of swblock kva space. > --- conf/options.i386 Mon Dec 10 04:17:04 2001 > +++ conf/options.i386 Wed May 29 11:32:38 2002 > @@ -30,6 +30,9 @@ > COMPAT_SVR4 opt_dontuse.h > DEBUG_SVR4 opt_svr4.h > > +# Kernel stack guard > +KSTACK_GUARD opt_global.h > + > # i386 SMP options > APIC_IO opt_global.h > > > To Unsubscribe: send mail to majordomo@FreeBSD.org > with "unsubscribe freebsd-arch" in the body of the message > To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message