From 83e044cf68d2d11ee96671d5c5492e4f804b8598 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 01:18:17 +0000 Subject: [PATCH 01/12] Let kernel handle its own page faults --- src/userprog/exception.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 0a20b53..8a800b7 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -145,6 +145,14 @@ page_fault (struct intr_frame *f) write = (f->error_code & PF_W) != 0; user = (f->error_code & PF_U) != 0; + /* Kernel page fault is further handled by the kernel itself. */ + if (!user) + { + f->eip = (void *)f->eax; + f->eax = 0xffffffff; + return; + } + /* To implement virtual memory, delete the rest of the function body, and replace it with code that brings in the page to which fault_addr refers. */ From 44f6a85163ee33cf09ec9180fea3a4f9a06b0e52 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 01:21:20 +0000 Subject: [PATCH 02/12] Add get_user and put_user provided by spec. --- src/userprog/syscall.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index ccb02f3..b162253 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -11,6 +11,7 @@ #include "userprog/process.h" #include "userprog/pagedir.h" #include +#include #include static struct lock filesys_lock; @@ -47,6 +48,8 @@ static void syscall_close (int fd); static struct open_file *fd_get_file (int fd); static void *validate_user_pointer (const void *ptr, size_t size); +static int get_user (const uint8_t *); +static bool put_user (uint8_t *, uint8_t); /* A struct defining a syscall_function pointer along with its arity. */ typedef struct @@ -415,3 +418,29 @@ validate_user_pointer (const void *ptr, size_t size) return (void *) ptr; } + +/* PROVIDED BY SPEC. + Reads a byte at user virtual address UADDR. + UADDR must be below PHYS_BASE. + Returns the byte value if successful, -1 if a segfault occurred. */ +static int +get_user (const uint8_t *uaddr) +{ + int result; + asm ("movl $1f, %0; movzbl %1, %0; 1:" : "=&a"(result) : "m"(*uaddr)); + return result; +} + +/* PROVIDED BY SPEC. + Writes BYTE to user address UDST. + UDST must be below PHYS_BASE. + Returns true if successful, false if a segfault occurred. */ +static bool +put_user (uint8_t *udst, uint8_t byte) +{ + int error_code; + asm ("movl $1f, %0; movb %b2, %1; 1:" + : "=&a"(error_code), "=m"(*udst) + : "q"(byte)); + return error_code != -1; +} \ No newline at end of file From 9a6abab95ed569c98cb7e0cf87b0428ee023de71 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 01:23:45 +0000 Subject: [PATCH 03/12] Check access to user memory using page fault method (via get_user and put_user). --- src/userprog/syscall.c | 52 ++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index b162253..ca6f116 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -47,7 +47,8 @@ static unsigned syscall_tell (int fd); static void syscall_close (int fd); static struct open_file *fd_get_file (int fd); -static void *validate_user_pointer (const void *ptr, size_t size); +static void *validate_user_pointer (const void *ptr, size_t size, + bool check_write); static int get_user (const uint8_t *); static bool put_user (uint8_t *, uint8_t); @@ -99,8 +100,8 @@ static void syscall_handler (struct intr_frame *f) { /* First, read the system call number from the stack. */ - validate_user_pointer (f->esp, 1); - unsigned syscall_number = *(int *) f->esp; + validate_user_pointer (f->esp, 1, false); + unsigned syscall_number = *(int *)f->esp; /* Ensures the number corresponds to a system call that can be handled. */ if (syscall_number >= LOOKUP_SIZE) @@ -110,10 +111,10 @@ syscall_handler (struct intr_frame *f) /* Next, read and copy the arguments from the stack pointer. */ validate_user_pointer (f->esp + sizeof (uintptr_t), - syscall.arity * sizeof (uintptr_t)); - uintptr_t args[3] = {0}; - for (int i=0; i < syscall.arity; i++) - args[i] = *(uintptr_t *) (f->esp + sizeof (uintptr_t) * (i + 1)); + syscall.arity * sizeof (uintptr_t), false); + uintptr_t args[3] = { 0 }; + for (int i = 0; i < syscall.arity; i++) + args[i] = *(uintptr_t *)(f->esp + sizeof (uintptr_t) * (i + 1)); /* Call the function that handles this system call with the arguments. When there is a return value it is stored in f->eax. */ @@ -144,7 +145,7 @@ syscall_exit (int status) static pid_t syscall_exec (const char *cmd_line) { - validate_user_pointer (cmd_line, 1); + validate_user_pointer (cmd_line, 1, false); lock_acquire (&filesys_lock); pid_t pid = process_execute(cmd_line); @@ -167,7 +168,7 @@ syscall_wait (pid_t pid) static bool syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) { - validate_user_pointer (file, 1); + validate_user_pointer (file, 1, false); lock_acquire (&filesys_lock); bool status = filesys_create (file, initial_size); @@ -182,7 +183,7 @@ syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) static bool syscall_remove (const char *file) { - validate_user_pointer (file, 1); + validate_user_pointer (file, 1, false); lock_acquire (&filesys_lock); bool status = filesys_remove (file); @@ -198,7 +199,7 @@ syscall_remove (const char *file) static int syscall_open (const char *file) { - validate_user_pointer (file, 1); + validate_user_pointer (file, 1, false); lock_acquire (&filesys_lock); struct file *ptr = filesys_open (file); @@ -253,7 +254,7 @@ syscall_read (int fd, void *buffer, unsigned size) if (fd < 0 || fd == STDOUT_FILENO) return -1; - validate_user_pointer (buffer, size); + validate_user_pointer (buffer, size, true); if (fd == STDIN_FILENO) { @@ -290,7 +291,7 @@ syscall_write (int fd, const void *buffer, unsigned size) if (fd <= 0) return 0; - validate_user_pointer (buffer, size); + validate_user_pointer (buffer, size, false); if (fd == STDOUT_FILENO) { @@ -404,19 +405,26 @@ fd_get_file (int fd) } /* Validates if a block of memory starting at PTR and of size SIZE bytes is - fully contained within user virtual memory. Kills the thread (by calling - thread_exit) if the memory is invalid. Otherwise, returns the PTR given. + fully contained within user virtual memory. Returns NULL if the memory + is invalid. Otherwise, returns the PTR given. If the size is 0, the function does no checks and returns PTR.*/ static void * -validate_user_pointer (const void *ptr, size_t size) +validate_user_pointer (const void *ptr, size_t size, bool check_write) { - if (size > 0 && (ptr == NULL || - !is_user_vaddr (ptr) || - !is_user_vaddr (ptr + size - 1) || - pagedir_get_page (thread_current()->pagedir, ptr) == NULL)) + if (size == 0) + return ptr; + /* ptr < ptr + size - 1, so sufficient to check that (ptr + size -1) is a + valid user virtual memory address. */ + if (!is_user_vaddr (ptr + size - 1)) thread_exit (); - - return (void *) ptr; + /* Check read access to pointer. */ + int result; + if ((result = get_user (ptr)) == -1) + thread_exit (); + /* Check write access to pointer (if required). */ + if (check_write && !put_user (ptr, result)) + thread_exit (); + return ptr; } /* PROVIDED BY SPEC. From cf4bf90cbb06eaa90961bdeab41f83a99666a90a Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Tue, 12 Nov 2024 15:34:45 +0000 Subject: [PATCH 04/12] Implement user pointer checking for C strings --- src/userprog/syscall.c | 74 ++++++++++++++++++++++++++++-------------- 1 file changed, 50 insertions(+), 24 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index ca6f116..2b0a551 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -14,6 +14,8 @@ #include #include +#define MAX_SYSCALL_ARGS 3 + static struct lock filesys_lock; static unsigned fd_counter = MIN_USER_FD; @@ -47,8 +49,9 @@ static unsigned syscall_tell (int fd); static void syscall_close (int fd); static struct open_file *fd_get_file (int fd); -static void *validate_user_pointer (const void *ptr, size_t size, - bool check_write); +static void validate_user_pointer (const void *ptr, size_t size, + bool check_write); +static void validate_user_string (const char *str, bool check_write); static int get_user (const uint8_t *); static bool put_user (uint8_t *, uint8_t); @@ -100,8 +103,8 @@ static void syscall_handler (struct intr_frame *f) { /* First, read the system call number from the stack. */ - validate_user_pointer (f->esp, 1, false); - unsigned syscall_number = *(int *)f->esp; + validate_user_pointer (f->esp, sizeof (uintptr_t), false); + uintptr_t syscall_number = *(int *)f->esp; /* Ensures the number corresponds to a system call that can be handled. */ if (syscall_number >= LOOKUP_SIZE) @@ -112,8 +115,8 @@ syscall_handler (struct intr_frame *f) /* Next, read and copy the arguments from the stack pointer. */ validate_user_pointer (f->esp + sizeof (uintptr_t), syscall.arity * sizeof (uintptr_t), false); - uintptr_t args[3] = { 0 }; - for (int i = 0; i < syscall.arity; i++) + uintptr_t args[MAX_SYSCALL_ARGS] = { 0 }; + for (int i = 0; i < syscall.arity && i < MAX_SYSCALL_ARGS; i++) args[i] = *(uintptr_t *)(f->esp + sizeof (uintptr_t) * (i + 1)); /* Call the function that handles this system call with the arguments. When @@ -145,7 +148,7 @@ syscall_exit (int status) static pid_t syscall_exec (const char *cmd_line) { - validate_user_pointer (cmd_line, 1, false); + validate_user_string (cmd_line, false); lock_acquire (&filesys_lock); pid_t pid = process_execute(cmd_line); @@ -168,7 +171,7 @@ syscall_wait (pid_t pid) static bool syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) { - validate_user_pointer (file, 1, false); + validate_user_string (file, false); lock_acquire (&filesys_lock); bool status = filesys_create (file, initial_size); @@ -183,7 +186,7 @@ syscall_create (const char *file UNUSED, unsigned initial_size UNUSED) static bool syscall_remove (const char *file) { - validate_user_pointer (file, 1, false); + validate_user_string (file, false); lock_acquire (&filesys_lock); bool status = filesys_remove (file); @@ -199,7 +202,7 @@ syscall_remove (const char *file) static int syscall_open (const char *file) { - validate_user_pointer (file, 1, false); + validate_user_string (file, false); lock_acquire (&filesys_lock); struct file *ptr = filesys_open (file); @@ -405,26 +408,49 @@ fd_get_file (int fd) } /* Validates if a block of memory starting at PTR and of size SIZE bytes is - fully contained within user virtual memory. Returns NULL if the memory - is invalid. Otherwise, returns the PTR given. - If the size is 0, the function does no checks and returns PTR.*/ -static void * + fully contained within valid user virtual memory. thread_exit () if the + memory is invalid. + If the size is 0, the function does no checks and returns PTR. */ +static void validate_user_pointer (const void *ptr, size_t size, bool check_write) { if (size == 0) - return ptr; + return; /* ptr < ptr + size - 1, so sufficient to check that (ptr + size -1) is a valid user virtual memory address. */ - if (!is_user_vaddr (ptr + size - 1)) + void *last = ptr + size - 1; + if (!is_user_vaddr (last)) thread_exit (); - /* Check read access to pointer. */ - int result; - if ((result = get_user (ptr)) == -1) - thread_exit (); - /* Check write access to pointer (if required). */ - if (check_write && !put_user (ptr, result)) - thread_exit (); - return ptr; + for (; ptr <= last; ptr++) + { + int result; + /* Check read access to pointer. */ + if ((result = get_user (ptr)) == -1) + thread_exit (); + /* Check write access to pointer (if required). */ + if (check_write && !put_user (ptr, result)) + thread_exit (); + } +} + +/* Validates of a C-string starting at ptr is fully contained within valid + user virtual memory. thread_exit () if the memory is invalid. */ +static void +validate_user_string (const char *ptr, bool check_write) +{ + while (true) + { + if (!is_user_vaddr (ptr)) + thread_exit (); + int result; + if ((result = get_user ((const uint8_t *)ptr)) == -1) + thread_exit (); + if (check_write && !put_user ((uint8_t *)ptr, result)) + thread_exit (); + if (*ptr == '\0') + return; + ptr++; + } } /* PROVIDED BY SPEC. From 59e7a64f8e1e538c010cf6c31974c4bf9d5dcbe1 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Tue, 12 Nov 2024 15:48:22 +0000 Subject: [PATCH 05/12] Only check user pages rather than all bytes in-between, for known-size pointers --- src/userprog/syscall.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 2b0a551..1be6c77 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -421,7 +421,8 @@ validate_user_pointer (const void *ptr, size_t size, bool check_write) void *last = ptr + size - 1; if (!is_user_vaddr (last)) thread_exit (); - for (; ptr <= last; ptr++) + ptr = pg_round_down (ptr); + while (ptr <= last) { int result; /* Check read access to pointer. */ @@ -430,6 +431,7 @@ validate_user_pointer (const void *ptr, size_t size, bool check_write) /* Check write access to pointer (if required). */ if (check_write && !put_user (ptr, result)) thread_exit (); + ptr += PGSIZE; } } From 3ef5264b6e3858b6e89311795a7f5e05ec02cb6a Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Tue, 26 Nov 2024 04:43:25 +0000 Subject: [PATCH 06/12] feat: allow stack to grow for process up to 8MB in size --- src/Makefile.build | 1 + src/userprog/exception.c | 7 +++++++ src/vm/stackgrowth.c | 38 ++++++++++++++++++++++++++++++++++++++ src/vm/stackgrowth.h | 11 +++++++++++ 4 files changed, 57 insertions(+) create mode 100644 src/vm/stackgrowth.c create mode 100644 src/vm/stackgrowth.h diff --git a/src/Makefile.build b/src/Makefile.build index c0d535e..4167f37 100644 --- a/src/Makefile.build +++ b/src/Makefile.build @@ -63,6 +63,7 @@ userprog_SRC += userprog/tss.c # TSS management. # Virtual memory code. vm_SRC += devices/swap.c # Swap block manager. +vm_SRC += vm/stackgrowth.c #vm_SRC = vm/file.c # Some other file. # Filesystem code. diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 0a20b53..500cb89 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -4,6 +4,7 @@ #include "userprog/gdt.h" #include "threads/interrupt.h" #include "threads/thread.h" +#include "vm/stackgrowth.h" /* Number of page faults processed. */ static long long page_fault_cnt; @@ -145,6 +146,12 @@ page_fault (struct intr_frame *f) write = (f->error_code & PF_W) != 0; user = (f->error_code & PF_U) != 0; + if (user && needs_new_page (fault_addr, f->esp)) + { + if (grow_stack (fault_addr)) + return; + } + /* To implement virtual memory, delete the rest of the function body, and replace it with code that brings in the page to which fault_addr refers. */ diff --git a/src/vm/stackgrowth.c b/src/vm/stackgrowth.c new file mode 100644 index 0000000..164eb9d --- /dev/null +++ b/src/vm/stackgrowth.c @@ -0,0 +1,38 @@ +#include +#include "stackgrowth.h" +#include "threads/palloc.h" +#include "threads/thread.h" +#include "threads/vaddr.h" +#include "userprog/pagedir.h" + +/* Validates a given address for being <=32 bytes away from the stack pointer or + above the stack */ +bool needs_new_page (void *addr, void *esp) +{ + return (is_user_vaddr (addr) && + (uint32_t*)addr >= ((uint32_t*)esp - 32) && + ((PHYS_BASE - pg_round_down (addr)) + <= MAX_STACK_SIZE)); +} + +/* Extends the stack by the necessary number of pages */ +bool grow_stack (void *addr) +{ + struct thread *t = thread_current (); + void *last_page = pg_round_down (addr); + + uint8_t *new_page = palloc_get_page (PAL_USER | PAL_ZERO); + if ( new_page == NULL) + return false; + + bool added_page = pagedir_get_page (t->pagedir, last_page) == NULL + && pagedir_set_page (t->pagedir, last_page, new_page, true); + + if (!added_page) { + palloc_free_page (new_page); + return false; + } + return true; + + +} \ No newline at end of file diff --git a/src/vm/stackgrowth.h b/src/vm/stackgrowth.h new file mode 100644 index 0000000..0502210 --- /dev/null +++ b/src/vm/stackgrowth.h @@ -0,0 +1,11 @@ +#ifndef GROWSTACK_H +#define GROWSTACK_H + +#include + +#define MAX_STACK_SIZE 8388608 // (8MB) + +bool needs_new_page (void *addr, void *esp); +bool grow_stack (void *addr); + +#endif //GROWSTACK_H From af7f2ba873e482ea210ca737e5b79da204f1d4ab Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Tue, 26 Nov 2024 04:54:00 +0000 Subject: [PATCH 07/12] Fix: Magic number in stackgrowth.c --- src/vm/stackgrowth.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/vm/stackgrowth.c b/src/vm/stackgrowth.c index 164eb9d..7d5470d 100644 --- a/src/vm/stackgrowth.c +++ b/src/vm/stackgrowth.c @@ -5,14 +5,16 @@ #include "threads/vaddr.h" #include "userprog/pagedir.h" +#define MAX_STACK_ACCESS_DIST 32 + /* Validates a given address for being <=32 bytes away from the stack pointer or above the stack */ bool needs_new_page (void *addr, void *esp) { return (is_user_vaddr (addr) && - (uint32_t*)addr >= ((uint32_t*)esp - 32) && - ((PHYS_BASE - pg_round_down (addr)) - <= MAX_STACK_SIZE)); + (uint32_t*)addr >= ((uint32_t*)esp - MAX_STACK_ACCESS_DIST) && + ((PHYS_BASE - pg_round_down (addr)) + <= MAX_STACK_SIZE)); } /* Extends the stack by the necessary number of pages */ From c670c29e47c009c4ccb0c9bb02bba06bb300f878 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Wed, 27 Nov 2024 18:57:20 +0000 Subject: [PATCH 08/12] update stack growth header to fit virtual memory naming format --- src/vm/stackgrowth.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vm/stackgrowth.h b/src/vm/stackgrowth.h index 0502210..a23e481 100644 --- a/src/vm/stackgrowth.h +++ b/src/vm/stackgrowth.h @@ -1,5 +1,5 @@ -#ifndef GROWSTACK_H -#define GROWSTACK_H +#ifndef VM_GROWSTACK_H +#define VM_GROWSTACK_H #include @@ -8,4 +8,4 @@ bool needs_new_page (void *addr, void *esp); bool grow_stack (void *addr); -#endif //GROWSTACK_H +#endif /* vm/frame.h */ From c74a8c55aae4d4607670b9d0f192a3cc891acd95 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Wed, 27 Nov 2024 19:21:43 +0000 Subject: [PATCH 09/12] Implement stack growth for system calls and add stack pointer tracking to thread --- src/threads/thread.h | 4 ++++ src/userprog/syscall.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/threads/thread.h b/src/threads/thread.h index 4a88577..60f91ce 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -143,6 +143,10 @@ struct thread struct hash open_files; /* Hash Table of FD -> Struct File. */ #endif +#ifdef VM + void *curr_esp; +#endif + /* Owned by thread.c. */ unsigned magic; /* Detects stack overflow. */ }; diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 3efe7b5..7fbc939 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -10,6 +10,9 @@ #include "threads/synch.h" #include "userprog/process.h" #include "userprog/pagedir.h" +#ifdef VM +#include "vm/stackgrowth.h" +#endif #include #include @@ -98,6 +101,7 @@ syscall_handler (struct intr_frame *f) /* First, read the system call number from the stack. */ validate_user_pointer (f->esp, sizeof (uintptr_t)); uintptr_t syscall_number = *(int *) f->esp; + thread_current ()->curr_esp = f->esp; /* Ensures the number corresponds to a system call that can be handled. */ if (syscall_number >= LOOKUP_SIZE) @@ -451,6 +455,20 @@ fd_get_file (int fd) return hash_entry (e, struct open_file, elem); } +static bool +try_alloc_new_page (const void *ptr) +{ + if (needs_new_page (ptr, thread_current()->curr_esp)) + { + if (!grow_stack (ptr)) + return 0; + else + return 1; + } + else + return 0; +} + /* Validates if a block of memory starting at START and of size SIZE bytes is fully contained within user virtual memory. Kills the thread (by exiting with failure) if the memory is invalid. Otherwise, returns (nothing) normally. @@ -472,7 +490,10 @@ validate_user_pointer (const void *start, size_t size) memory by the page table. */ for (const void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE) if (pagedir_get_page (thread_current ()->pagedir, ptr) == NULL) - syscall_exit (EXIT_FAILURE); + { + if (!try_alloc_new_page (ptr)) + syscall_exit (EXIT_FAILURE); + } } /* Validates if a string is fully contained within user virtual memory. Kills @@ -495,10 +516,13 @@ validate_user_string (const char *str) /* If we reach addresses that are not mapped to physical memory before the end of the string, the thread is terminated. */ - if (!is_user_vaddr(page) || - pagedir_get_page (thread_current ()->pagedir, page) == NULL) + if (!is_user_vaddr(page)) syscall_exit (EXIT_FAILURE); + if (pagedir_get_page (thread_current ()->pagedir, page) == NULL) + if (!try_alloc_new_page (str)) + syscall_exit (EXIT_FAILURE); + while (offset < PGSIZE) { if (*str == '\0') From 4f84a83611aad4bde85641d83c5a7d00f2ec051e Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Wed, 27 Nov 2024 19:41:22 +0000 Subject: [PATCH 10/12] Refactor: abstract new page allocation to one general function and make helper functions static --- src/userprog/exception.c | 4 ++-- src/userprog/syscall.c | 25 +++++++------------------ src/vm/stackgrowth.c | 30 ++++++++++++++++++++++++------ src/vm/stackgrowth.h | 3 +-- 4 files changed, 34 insertions(+), 28 deletions(-) diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 500cb89..272325e 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -146,9 +146,9 @@ page_fault (struct intr_frame *f) write = (f->error_code & PF_W) != 0; user = (f->error_code & PF_U) != 0; - if (user && needs_new_page (fault_addr, f->esp)) + if (user) { - if (grow_stack (fault_addr)) + if (try_alloc_new_page (fault_addr, f->esp)) return; } diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 7fbc939..26a37e7 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -455,20 +455,6 @@ fd_get_file (int fd) return hash_entry (e, struct open_file, elem); } -static bool -try_alloc_new_page (const void *ptr) -{ - if (needs_new_page (ptr, thread_current()->curr_esp)) - { - if (!grow_stack (ptr)) - return 0; - else - return 1; - } - else - return 0; -} - /* Validates if a block of memory starting at START and of size SIZE bytes is fully contained within user virtual memory. Kills the thread (by exiting with failure) if the memory is invalid. Otherwise, returns (nothing) normally. @@ -480,6 +466,8 @@ validate_user_pointer (const void *start, size_t size) if (size == 0) return; + struct thread *t = thread_current (); + const void *end = start + size - 1; /* Check if the start and end pointers are valid user virtual addresses. */ @@ -489,9 +477,9 @@ validate_user_pointer (const void *start, size_t size) /* We now need to check if the entire memory block is mapped to physical memory by the page table. */ for (const void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE) - if (pagedir_get_page (thread_current ()->pagedir, ptr) == NULL) + if (pagedir_get_page (t->pagedir, ptr) == NULL) { - if (!try_alloc_new_page (ptr)) + if (!try_alloc_new_page (ptr, t->curr_esp)) syscall_exit (EXIT_FAILURE); } } @@ -508,6 +496,7 @@ validate_user_string (const char *str) /* Calculate the offset of the string within the (first) page. */ size_t offset = (uintptr_t) str % PGSIZE; + struct thread *t = thread_current (); /* We move page by page, checking if the page is mapped to physical memory. */ for (;;) @@ -519,8 +508,8 @@ validate_user_string (const char *str) if (!is_user_vaddr(page)) syscall_exit (EXIT_FAILURE); - if (pagedir_get_page (thread_current ()->pagedir, page) == NULL) - if (!try_alloc_new_page (str)) + if (pagedir_get_page (t->pagedir, page) == NULL) + if (!try_alloc_new_page (str, t->curr_esp)) syscall_exit (EXIT_FAILURE); while (offset < PGSIZE) diff --git a/src/vm/stackgrowth.c b/src/vm/stackgrowth.c index 7d5470d..50bdc39 100644 --- a/src/vm/stackgrowth.c +++ b/src/vm/stackgrowth.c @@ -7,9 +7,28 @@ #define MAX_STACK_ACCESS_DIST 32 -/* Validates a given address for being <=32 bytes away from the stack pointer or - above the stack */ -bool needs_new_page (void *addr, void *esp) +static bool needs_new_page (const void *addr, const void *esp); +static bool grow_stack (const void *addr); + +bool +try_alloc_new_page (const void *ptr, const void *esp) +{ + if (needs_new_page (ptr, esp)) + { + if (!grow_stack (ptr)) + return 0; + else + return 1; + } + else + return 0; +} + +/* Validates a given address for being a stack query and not a generic erroneous + address + */ +static bool +needs_new_page (const void *addr, const void *esp) { return (is_user_vaddr (addr) && (uint32_t*)addr >= ((uint32_t*)esp - MAX_STACK_ACCESS_DIST) && @@ -18,7 +37,8 @@ bool needs_new_page (void *addr, void *esp) } /* Extends the stack by the necessary number of pages */ -bool grow_stack (void *addr) +static bool +grow_stack (const void *addr) { struct thread *t = thread_current (); void *last_page = pg_round_down (addr); @@ -35,6 +55,4 @@ bool grow_stack (void *addr) return false; } return true; - - } \ No newline at end of file diff --git a/src/vm/stackgrowth.h b/src/vm/stackgrowth.h index a23e481..acd123e 100644 --- a/src/vm/stackgrowth.h +++ b/src/vm/stackgrowth.h @@ -5,7 +5,6 @@ #define MAX_STACK_SIZE 8388608 // (8MB) -bool needs_new_page (void *addr, void *esp); -bool grow_stack (void *addr); +bool try_alloc_new_page (const void *ptr, const void *esp); #endif /* vm/frame.h */ From 5c661c2e24bc83c1229516ea193b3c87237c2aa5 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 29 Nov 2024 23:49:49 +0000 Subject: [PATCH 11/12] Feat: pointer validation checks string across multiple pages and handle kernel page faults --- src/threads/thread.h | 2 -- src/userprog/Make.vars | 4 ++-- src/userprog/exception.c | 20 ++++++++++---------- src/userprog/syscall.c | 38 +++++++++++++++++++++++++------------- 4 files changed, 37 insertions(+), 27 deletions(-) diff --git a/src/threads/thread.h b/src/threads/thread.h index 60f91ce..f031981 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -143,9 +143,7 @@ struct thread struct hash open_files; /* Hash Table of FD -> Struct File. */ #endif -#ifdef VM void *curr_esp; -#endif /* Owned by thread.c. */ unsigned magic; /* Detects stack overflow. */ diff --git a/src/userprog/Make.vars b/src/userprog/Make.vars index e4dbb08..23bae3d 100644 --- a/src/userprog/Make.vars +++ b/src/userprog/Make.vars @@ -1,7 +1,7 @@ # -*- makefile -*- -kernel.bin: DEFINES = -DUSERPROG -DFILESYS -KERNEL_SUBDIRS = threads devices lib lib/kernel userprog filesys +kernel.bin: DEFINES = -DUSERPROG -DFILESYS -DVM +KERNEL_SUBDIRS = threads devices lib lib/kernel userprog filesys vm TEST_SUBDIRS = tests/userprog tests/userprog/no-vm tests/filesys/base GRADING_FILE = $(SRCDIR)/tests/userprog/Grading SIMULATOR = --qemu diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 3e3b133..1fcbe61 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -146,19 +146,19 @@ page_fault (struct intr_frame *f) write = (f->error_code & PF_W) != 0; user = (f->error_code & PF_U) != 0; - /* Kernel page fault is further handled by the kernel itself. */ - if (kernel) + if (user && not_present) { - f->eip = (void *)f->eax; + if (try_alloc_new_page (fault_addr, f->esp)) + return; + } + else + { + if (try_alloc_new_page (fault_addr, thread_current ()->curr_esp)) + return; + f->eip = (void *)f->eax; f->eax = 0xffffffff; return; - } - - if (user) - { - if (try_alloc_new_page (fault_addr, f->esp)) - return; - } + } /* To implement virtual memory, delete the rest of the function body, and replace it with code that brings in the page to diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index da6d77d..4bc34ca 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -10,9 +10,6 @@ #include "threads/synch.h" #include "userprog/process.h" #include "userprog/pagedir.h" -#ifdef VM -#include "vm/stackgrowth.h" -#endif #include #include #include @@ -465,17 +462,17 @@ validate_user_pointer (const void *ptr, size_t size, bool check_write) valid user virtual memory address. */ void *last = ptr + size - 1; if (!is_user_vaddr (last)) - thread_exit (); + syscall_exit (EXIT_FAILURE); ptr = pg_round_down (ptr); while (ptr <= last) { int result; /* Check read access to pointer. */ if ((result = get_user (ptr)) == -1) - thread_exit (); + syscall_exit (EXIT_FAILURE); /* Check write access to pointer (if required). */ if (check_write && !put_user (ptr, result)) - thread_exit (); + syscall_exit (EXIT_FAILURE); ptr += PGSIZE; } } @@ -485,18 +482,33 @@ validate_user_pointer (const void *ptr, size_t size, bool check_write) static void validate_user_string (const char *ptr, bool check_write) { - while (true) + size_t offset = (uintptr_t) ptr % PGSIZE; + + for (;;) { + void *page = pg_round_down (ptr); + + if (!is_user_vaddr (page)) + syscall_exit (EXIT_FAILURE); if (!is_user_vaddr (ptr)) - thread_exit (); + syscall_exit (EXIT_FAILURE); int result; if ((result = get_user ((const uint8_t *)ptr)) == -1) - thread_exit (); + syscall_exit (EXIT_FAILURE); if (check_write && !put_user ((uint8_t *)ptr, result)) - thread_exit (); - if (*ptr == '\0') - return; - ptr++; + syscall_exit (EXIT_FAILURE); + + while (offset < PGSIZE) + { + if (*ptr == '\0') + return; /* We reached the end of the string without issues. */ + + ptr++; + offset++; + } + + offset = 0; + } } From 13de832586e55a4899e0aa1b09abb97ddd98a571 Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 29 Nov 2024 23:52:05 +0000 Subject: [PATCH 12/12] Refactor stack growth code to remove messy conditions --- src/vm/stackgrowth.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/vm/stackgrowth.c b/src/vm/stackgrowth.c index 50bdc39..bc9717c 100644 --- a/src/vm/stackgrowth.c +++ b/src/vm/stackgrowth.c @@ -13,15 +13,7 @@ static bool grow_stack (const void *addr); bool try_alloc_new_page (const void *ptr, const void *esp) { - if (needs_new_page (ptr, esp)) - { - if (!grow_stack (ptr)) - return 0; - else - return 1; - } - else - return 0; + return needs_new_page (ptr, esp) && grow_stack (ptr); } /* Validates a given address for being a stack query and not a generic erroneous