diff --git a/kernel/include/sys/UserMemory.h b/kernel/include/sys/UserMemory.h index 0da06379..3ce99d3f 100644 --- a/kernel/include/sys/UserMemory.h +++ b/kernel/include/sys/UserMemory.h @@ -8,8 +8,13 @@ #include "memory/MemoryManager.h" #include "memory/VMM.h" #include "misc/utils.h" +#include char* strdup_from_user(const char* user_string); +bool validate_user_readable_page(uintptr_t address); +bool validate_user_writable_page(uintptr_t address); +bool validate_user_read(uintptr_t address, size_t size); +bool validate_user_write(uintptr_t address, size_t size); // FIXME: Map the physical addresses into kernel address space. Right now, something overwrites KernelHeap and crashes // it, so that's not really possible. But it should be done in the future. diff --git a/kernel/src/sys/UserMemory.cpp b/kernel/src/sys/UserMemory.cpp index 6ca659ff..c2b7b9a5 100644 --- a/kernel/src/sys/UserMemory.cpp +++ b/kernel/src/sys/UserMemory.cpp @@ -1,5 +1,7 @@ #include "sys/UserMemory.h" +#include "memory/Memory.h" #include "std/string.h" +#include "utils/Addresses.h" char* strdup_from_user( const char* user_string) // FIXME: This function is a little hacky. Use the obtain_user_ref and similar functions. @@ -7,4 +9,58 @@ char* strdup_from_user( uint64_t phys = VMM::get_physical((uint64_t)user_string); if (phys == (uint64_t)-1) { return nullptr; } return strdup((const char*)phys); +} + +bool validate_user_readable_page(uintptr_t address) +{ + if (Memory::is_kernel_address(address)) return false; + auto rc = VMM::get_flags(address); + if (rc == (uint64_t)-1) return false; + if (rc & MAP_USER) return true; + return false; +} + +bool validate_user_writable_page(uintptr_t address) +{ + if (Memory::is_kernel_address(address)) return false; + auto rc = VMM::get_flags(address); + if (rc == (uint64_t)-1) return false; + if (rc & (MAP_USER | MAP_READ_WRITE)) return true; + return false; +} + +bool validate_user_read(uintptr_t address, size_t size) +{ + auto aligned = round_down_to_nearest_page(address); + if (aligned != address) // Otherwise, we already do this check below. + { + if (!validate_user_readable_page(aligned)) return false; + } + while (size--) + { + if (address % PAGE_SIZE == 0) + { + if (!validate_user_readable_page(address)) return false; + } + address++; + } + return true; +} + +bool validate_user_write(uintptr_t address, size_t size) +{ + auto aligned = round_down_to_nearest_page(address); + if (aligned != address) // Otherwise, we already do this check below. + { + if (!validate_user_writable_page(aligned)) return false; + } + while (size--) + { + if (address % PAGE_SIZE == 0) + { + if (!validate_user_writable_page(address)) return false; + } + address++; + } + return true; } \ No newline at end of file diff --git a/kernel/src/sys/exec.cpp b/kernel/src/sys/exec.cpp index 6e96faa1..391c25fa 100644 --- a/kernel/src/sys/exec.cpp +++ b/kernel/src/sys/exec.cpp @@ -131,15 +131,17 @@ void sys_execv(Context* context, const char* pathname, char** argv) if (kargv) kfree(kargv); }; + // FIXME: This is a bit messy. + do { - if (Memory::is_kernel_address((uintptr_t)argv)) + if (!validate_user_read((uintptr_t)argv, sizeof(char*))) { free_kernel_argv_copy(); context->rax = -EFAULT; return; } - arg = (char**)VMM::get_physical((uint64_t)argv); - if (arg == (char**)UINT64_MAX) + arg = obtain_user_ref(argv); + if (!arg) // This should never happen, since it was already checked just before. { free_kernel_argv_copy(); context->rax = -EFAULT; @@ -148,6 +150,7 @@ void sys_execv(Context* context, const char* pathname, char** argv) kargv = (char**)krealloc(kargv, (kargc + 1) * sizeof(char*)); if (!kargv) { + release_user_ref(arg); free_kernel_argv_copy(); context->rax = -ENOMEM; return; @@ -157,6 +160,7 @@ void sys_execv(Context* context, const char* pathname, char** argv) char* kcopy = strdup_from_user(*arg); if (!kcopy) { + release_user_ref(arg); free_kernel_argv_copy(); context->rax = -ENOMEM; return; @@ -168,6 +172,7 @@ void sys_execv(Context* context, const char* pathname, char** argv) kargv[kargc] = nullptr; break; } + release_user_ref(arg); kargc++; argv++; } while (arg != nullptr); @@ -216,8 +221,8 @@ void sys_execv(Context* context, const char* pathname, char** argv) task->address_space.clear(); task->allocated_stack = (uint64_t)MemoryManager::get_pages_at( 0x100000, TASK_PAGES_IN_STACK, - MAP_USER | MAP_READ_WRITE | MAP_AS_OWNED_BY_TASK); // If we had enough space for the old stack, there should be enough space for the - // new stack. + MAP_USER | MAP_READ_WRITE | MAP_AS_OWNED_BY_TASK); // If we had enough space for the old stack, there should be + // enough space for the new stack. ELFImage* image = ELFLoader::load_elf_from_vfs(program); ensure(image); // If check_elf_image succeeded, load_elf_from_vfs MUST succeed, unless something has gone terribly diff --git a/kernel/src/sys/stdio.cpp b/kernel/src/sys/stdio.cpp index 8c1a735a..d4e85bfb 100644 --- a/kernel/src/sys/stdio.cpp +++ b/kernel/src/sys/stdio.cpp @@ -138,7 +138,7 @@ void sys_seek(Context* context, int fd, long offset, int whence) void sys_write(Context* context, int fd, size_t size, const char* addr) { - if (!addr) + if (!validate_user_read((uintptr_t)addr, size)) { context->rax = -EFAULT; return; @@ -156,7 +156,8 @@ void sys_write(Context* context, int fd, size_t size, const char* addr) return; } ssize_t result = file->write( - size, (const char*)VMM::get_physical((uint64_t)addr)); // FIXME: Handle big buffers and invalid addresses. + size, (const char*)VMM::get_physical((uint64_t)addr)); // FIXME: Handle buffers bigger than one page, which may + // not be contiguous in physical memory. context->rax = (size_t)result; return; } @@ -269,7 +270,7 @@ void sys_open(Context* context, const char* filename, int flags, mode_t) // FIXM void sys_read(Context* context, int fd, size_t size, char* buffer) { - if (!buffer) + if (!validate_user_write((uintptr_t)buffer, size)) { context->rax = -EFAULT; return; @@ -302,7 +303,7 @@ void sys_read(Context* context, int fd, size_t size, char* buffer) return Scheduler::task_yield(context); } ssize_t result = - file->read(size, (char*)VMM::get_physical((uint64_t)buffer)); // FIXME: Handle errors, and big buffers which may + file->read(size, (char*)VMM::get_physical((uint64_t)buffer)); // FIXME: Handle big buffers which may // not be across continuous physical pages. context->rax = (size_t)result; return;