From c4cc10da7ef6d65f0f654445e0af35b8309f16c2 Mon Sep 17 00:00:00 2001 From: Robert Morris Date: Fri, 6 Aug 2010 11:12:18 -0400 Subject: [PATCH] fix corner cases in exec of ELF put an invalid page below the stack have fork() handle invalid pages --- defs.h | 3 ++- exec.c | 7 ++++-- kalloc.c | 5 ++-- mmu.h | 1 - proc.c | 10 ++++---- proc.h | 5 ++-- usertests.c | 24 +++++++++++++++++++ vm.c | 66 ++++++++++++++++++++++++++++++++++------------------- 8 files changed, 84 insertions(+), 37 deletions(-) diff --git a/defs.h b/defs.h index 4a63154..b691099 100644 --- a/defs.h +++ b/defs.h @@ -163,7 +163,8 @@ void freevm(pde_t*); void inituvm(pde_t*, char*, char*, uint); int loaduvm(pde_t*, char*, struct inode *ip, uint, uint); pde_t* copyuvm(pde_t*,uint); -void loadvm(struct proc*); +void switchuvm(struct proc*); +void switchkvm(); // number of elements in fixed-size array #define NELEM(x) (sizeof(x)/sizeof((x)[0])) diff --git a/exec.c b/exec.c index 8a92e99..4f11695 100644 --- a/exec.c +++ b/exec.c @@ -43,13 +43,16 @@ exec(char *path, char **argv) goto bad; if (!allocuvm(pgdir, (char *)ph.va, ph.memsz)) goto bad; - sz += PGROUNDUP(ph.memsz); + if(ph.va + ph.memsz > sz) + sz = ph.va + ph.memsz; if (!loaduvm(pgdir, (char *)ph.va, ip, ph.offset, ph.filesz)) goto bad; } iunlockput(ip); // Allocate and initialize stack at sz + sz = PGROUNDUP(sz); + sz += PGSIZE; // leave an invalid page if (!allocuvm(pgdir, (char *)sz, PGSIZE)) goto bad; mem = uva2ka(pgdir, (char *)sz); @@ -95,7 +98,7 @@ exec(char *path, char **argv) proc->tf->eip = elf.entry; // main proc->tf->esp = sp; - loadvm(proc); + switchuvm(proc); freevm(oldpgdir); diff --git a/kalloc.c b/kalloc.c index 5661105..7695006 100644 --- a/kalloc.c +++ b/kalloc.c @@ -1,9 +1,8 @@ // Physical memory allocator, intended to allocate -// memory for user processes. Allocates in 4096-byte "pages". +// memory for user processes. Allocates in 4096-byte pages. // Free list is kept sorted and combines adjacent pages into // long runs, to make it easier to allocate big segments. -// One reason the page size is 4k is that the x86 segment size -// granularity is 4k. +// This combining is not useful now that xv6 uses paging. #include "types.h" #include "defs.h" diff --git a/mmu.h b/mmu.h index 76a6f1b..f4fc732 100644 --- a/mmu.h +++ b/mmu.h @@ -129,7 +129,6 @@ struct segdesc { #define PTE_ADDR(pte) ((uint) (pte) & ~0xFFF) typedef uint pte_t; -extern pde_t *kpgdir; // Control Register flags #define CR0_PE 0x00000001 // Protection Enable diff --git a/proc.c b/proc.c index dd6f27e..f799a4d 100644 --- a/proc.c +++ b/proc.c @@ -145,7 +145,7 @@ growproc(int n) if (!allocuvm(proc->pgdir, (char *)proc->sz, n)) return -1; proc->sz += n; - loadvm(proc); + switchuvm(proc); return 0; } @@ -214,9 +214,10 @@ scheduler(void) // to release ptable.lock and then reacquire it // before jumping back to us. proc = p; - loadvm(p); + switchuvm(p); p->state = RUNNING; swtch(&cpu->scheduler, proc->context); + switchkvm(); // Process is done running for now. // It should have changed its p->state before coming back. @@ -242,7 +243,6 @@ sched(void) panic("sched running"); if(readeflags()&FL_IF) panic("sched interruptible"); - lcr3(PADDR(kpgdir)); // Switch to the kernel page table intena = cpu->intena; swtch(&proc->context, cpu->scheduler); cpu->intena = intena; @@ -414,8 +414,8 @@ wait(void) // Found one. pid = p->pid; kfree(p->kstack, KSTACKSIZE); - p->kstack = 0; - freevm(p->pgdir); + p->kstack = 0; + freevm(p->pgdir); p->state = UNUSED; p->pid = 0; p->parent = 0; diff --git a/proc.h b/proc.h index 5e5d031..7d97dfa 100644 --- a/proc.h +++ b/proc.h @@ -16,7 +16,7 @@ // Contexts are stored at the bottom of the stack they // describe; the stack pointer is the address of the context. // The layout of the context matches the layout of the stack in swtch.S -// at "Switch stacks" comment. Switch itself doesn't save eip explicitly, +// at the "Switch stacks" comment. Switch doesn't save eip explicitly, // but it is on the stack and allocproc() manipulates it. struct context { uint edi; @@ -31,7 +31,7 @@ enum procstate { UNUSED, EMBRYO, SLEEPING, RUNNABLE, RUNNING, ZOMBIE }; // Per-process state struct proc { uint sz; // Size of process memory (bytes) - pde_t* pgdir; // linear address of proc's pgdir + pde_t* pgdir; // Linear address of proc's pgdir char *kstack; // Bottom of kernel stack for this process enum procstate state; // Process state volatile int pid; // Process ID @@ -48,6 +48,7 @@ struct proc { // Process memory is laid out contiguously, low addresses first: // text // original data and bss +// invalid page // fixed-size stack // expandable heap diff --git a/usertests.c b/usertests.c index 2bd21ba..247cc95 100644 --- a/usertests.c +++ b/usertests.c @@ -1261,6 +1261,29 @@ sbrktest(void) printf(stdout, "sbrk test OK\n"); } +void +stacktest(void) +{ + printf(stdout, "stack test\n"); + char dummy = 1; + char *p = &dummy; + int ppid = getpid(); + int pid = fork(); + if(pid < 0){ + printf(stdout, "fork failed\n"); + exit(); + } + if(pid == 0){ + // should cause a trap: + p[-4096] = 'z'; + kill(ppid); + printf(stdout, "stack test failed: page before stack was writeable\n"); + exit(); + } + wait(); + printf(stdout, "stack test OK\n"); +} + int main(int argc, char *argv[]) { @@ -1272,6 +1295,7 @@ main(int argc, char *argv[]) } close(open("usertests.ran", O_CREATE)); + stacktest(); sbrktest(); opentest(); diff --git a/vm.c b/vm.c index 9ea5e92..6914dd3 100644 --- a/vm.c +++ b/vm.c @@ -8,13 +8,20 @@ // The mappings from logical to linear are one to one (i.e., // segmentation doesn't do anything). -// The mapping from linear to physical are one to one for the kernel. -// The mappings for the kernel include all of physical memory (until -// PHYSTOP), including the I/O hole, and the top of physical address -// space, where additional devices are located. -// The kernel itself is linked to be at 1MB, and its physical memory -// is also at 1MB. -// Physical memory for user programs is allocated from physical memory +// There is one page table per process, plus one that's used +// when a CPU is not running any process (kpgdir). +// A user process uses the same page table as the kernel; the +// page protection bits prevent it from using anything other +// than its memory. +// +// setupkvm() and exec() set up every page table like this: +// 0..640K : user memory (text, data, stack, heap) +// 640K..1M : mapped direct (for IO space) +// 1M..kernend : mapped direct (for the kernel's text and data) +// kernend..PHYSTOP : mapped direct (kernel heap and user pages) +// 0xfe000000..0 : mapped direct (devices such as ioapic) +// +// The kernel allocates memory for its heap and for user memory // between kernend and the end of physical memory (PHYSTOP). // The virtual address space of each user program includes the kernel // (which is inaccessible in user mode). The user program addresses @@ -31,7 +38,7 @@ static uint kerndata; static uint kerndsz; static uint kernend; static uint freesz; -pde_t *kpgdir; // One kernel page table for scheduler procs +static pde_t *kpgdir; // for use in scheduler() // return the address of the PTE in page table pgdir // that corresponds to linear address va. if create!=0, @@ -114,9 +121,9 @@ ksegment(void) proc = 0; } -// Setup address space and current process task state. +// Switch h/w page table and TSS registers to point to process p. void -loadvm(struct proc *p) +switchuvm(struct proc *p) { pushcli(); @@ -128,14 +135,21 @@ loadvm(struct proc *p) ltr(SEG_TSS << 3); if (p->pgdir == 0) - panic("loadvm: no pgdir\n"); + panic("switchuvm: no pgdir\n"); lcr3(PADDR(p->pgdir)); // switch to new address space popcli(); } -// Setup kernel part of a page table. Linear adresses map one-to-one -// on physical addresses. +// Switch h/w page table register to the kernel-only page table, for when +// no process is running. +void +switchkvm() +{ + lcr3(PADDR(kpgdir)); // Switch to the kernel page table +} + +// Set up kernel part of a page table. pde_t* setupkvm(void) { @@ -163,6 +177,10 @@ setupkvm(void) return pgdir; } +// return the physical address that a given user address +// maps to. the result is also a kernel logical address, +// since the kernel maps the physical memory allocated to user +// processes directly. char* uva2ka(pde_t *pgdir, char *uva) { @@ -266,6 +284,8 @@ inituvm(pde_t *pgdir, char *addr, char *init, uint sz) } } +// given a parent process's page table, create a copy +// of it for a child. pde_t* copyuvm(pde_t *pgdir, uint sz) { @@ -278,17 +298,20 @@ copyuvm(pde_t *pgdir, uint sz) for (i = 0; i < sz; i += PGSIZE) { if (!(pte = walkpgdir(pgdir, (void *)i, 0))) panic("copyuvm: pte should exist\n"); - pa = PTE_ADDR(*pte); - if (!(mem = kalloc(PGSIZE))) - return 0; - memmove(mem, (char *)pa, PGSIZE); - if (!mappages(d, (void *)i, PGSIZE, PADDR(mem), PTE_W|PTE_U)) - return 0; + if(*pte & PTE_P){ + pa = PTE_ADDR(*pte); + if (!(mem = kalloc(PGSIZE))) + return 0; + memmove(mem, (char *)pa, PGSIZE); + if (!mappages(d, (void *)i, PGSIZE, PADDR(mem), PTE_W|PTE_U)) + return 0; + } } return d; } -// Gather about physical memory layout. Called once during boot. +// Gather information about physical memory layout. +// Called once during boot. void pminit(void) { @@ -307,9 +330,6 @@ pminit(void) kerndsz = ph[1].memsz; freesz = PHYSTOP - kernend; - cprintf("kerntext@0x%x(sz=0x%x), kerndata@0x%x(sz=0x%x), kernend 0x%x freesz = 0x%x\n", - kerntext, kerntsz, kerndata, kerndsz, kernend, freesz); - kinit((char *)kernend, freesz); }