fix corner cases in exec of ELF

put an invalid page below the stack have fork() handle invalid pages

fix corner cases in exec of ELF
c4cc10da · Robert Morris · 1afc9d3f · c4cc10da · c4cc10da · c4cc10da
--- 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]))
--- 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);

--- a/kalloc.c
+++ b/kalloc.c
 // 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
+// This combining is not useful now that xv6 uses paging.
-// granularity is 4k.
 #include "types.h"
 #include "defs.h"

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

--- 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;
+        p->kstack = 0;
-	freevm(p->pgdir);
+        freevm(p->pgdir);
        p->state = UNUSED;
        p->pid = 0;
        p->parent = 0;

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

--- 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();

--- 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.
+// There is one page table per process, plus one that's used
-// The mappings for the kernel include all of physical memory (until
+// when a CPU is not running any process (kpgdir).
-// PHYSTOP), including the I/O hole, and the top of physical address
+// A user process uses the same page table as the kernel; the
-// space, where additional devices are located.
+// page protection bits prevent it from using anything other
-// The kernel itself is linked to be at 1MB, and its physical memory
+// than its memory.
-// is also at 1MB.
+// 
-// Physical memory for user programs is allocated from physical 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
+// Switch h/w page table register to the kernel-only page table, for when
-// on physical addresses.
+// 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(*pte & PTE_P){
-    if (!(mem = kalloc(PGSIZE)))
+      pa = PTE_ADDR(*pte);
-      return 0;
+      if (!(mem = kalloc(PGSIZE)))
-    memmove(mem, (char *)pa, PGSIZE);
+        return 0;
-    if (!mappages(d, (void *)i, PGSIZE, PADDR(mem), PTE_W|PTE_U))
+      memmove(mem, (char *)pa, PGSIZE);
-      return 0;
+      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);
 }