diff --git a/kernel/elf.cpp b/kernel/elf.cpp index 4acc1b6d..7f12ccde 100644 --- a/kernel/elf.cpp +++ b/kernel/elf.cpp @@ -285,17 +285,20 @@ uintptr_t Load(const void* file_ptr, size_t file_size, Auxiliary* aux) assert(IsUserspaceSegment(&segment)); + kthread_mutex_lock(&process->segment_write_lock); kthread_mutex_lock(&process->segment_lock); if ( IsSegmentOverlapping(process, &segment) ) { kthread_mutex_unlock(&process->segment_lock); + kthread_mutex_unlock(&process->segment_write_lock); return errno = EINVAL, 0; } if ( !Memory::MapRange(segment.addr, segment.size, prot, PAGE_USAGE_USER_SPACE) ) { kthread_mutex_unlock(&process->segment_lock); + kthread_mutex_unlock(&process->segment_write_lock); return errno = EINVAL, 0; } @@ -303,10 +306,12 @@ uintptr_t Load(const void* file_ptr, size_t file_size, Auxiliary* aux) { Memory::UnmapRange(segment.addr, segment.size, PAGE_USAGE_USER_SPACE); kthread_mutex_unlock(&process->segment_lock); + kthread_mutex_unlock(&process->segment_write_lock); return errno = EINVAL, 0; } kthread_mutex_unlock(&process->segment_lock); + kthread_mutex_unlock(&process->segment_write_lock); memset((void*) segment.addr, 0, segment.size); memcpy((void*) pheader->p_vaddr, file + pheader->p_offset, pheader->p_filesz); diff --git a/kernel/include/sortix/kernel/process.h b/kernel/include/sortix/kernel/process.h index 2d987000..f87aa90e 100644 --- a/kernel/include/sortix/kernel/process.h +++ b/kernel/include/sortix/kernel/process.h @@ -153,6 +153,7 @@ public: struct segment* segments; size_t segments_used; size_t segments_length; + kthread_mutex_t segment_write_lock; kthread_mutex_t segment_lock; public: diff --git a/kernel/memorymanagement.cpp b/kernel/memorymanagement.cpp index 440be5ce..d764ecfa 100644 --- a/kernel/memorymanagement.cpp +++ b/kernel/memorymanagement.cpp @@ -63,6 +63,7 @@ namespace Memory { void UnmapMemory(Process* process, uintptr_t addr, size_t size) { + // process->segment_write_lock is held. // process->segment_lock is held. assert(Page::IsAligned(addr)); assert(Page::IsAligned(size)); @@ -133,6 +134,7 @@ void UnmapMemory(Process* process, uintptr_t addr, size_t size) bool ProtectMemory(Process* process, uintptr_t addr, size_t size, int prot) { + // process->segment_write_lock is held. // process->segment_lock is held. assert(Page::IsAligned(addr)); assert(Page::IsAligned(size)); @@ -224,6 +226,7 @@ bool ProtectMemory(Process* process, uintptr_t addr, size_t size, int prot) bool MapMemory(Process* process, uintptr_t addr, size_t size, int prot) { + // process->segment_write_lock is held. // process->segment_lock is held. assert(Page::IsAligned(addr)); assert(Page::IsAligned(size)); @@ -247,8 +250,8 @@ bool MapMemory(Process* process, uintptr_t addr, size_t size, int prot) return false; } - // We have process->segment_lock locked, so we know that the memory in user - // space exists and we can safely zero it here. + // We have process->segment_write_lock locked, so we know that the memory in + // user space exists and we can safely zero it here. // TODO: Another thread is able to see the old contents of the memory before // we zero it causing potential information leaks. // TODO: SECURITY: Information leak. @@ -335,7 +338,8 @@ void* sys_mmap(void* addr_ptr, size_t size, int prot, int flags, int fd, return errno = EACCES, MAP_FAILED; } - ScopedLock lock(&process->segment_lock); + ScopedLock lock1(&process->segment_write_lock); + ScopedLock lock2(&process->segment_lock); // Determine where to put the new segment and its protection. struct segment new_segment; @@ -351,20 +355,18 @@ void* sys_mmap(void* addr_ptr, size_t size, int prot, int flags, int fd, return MAP_FAILED; // The pread will copy to user-space right requires this lock to be free. - // TODO: This means another thread can concurrently change this memory - // mapping while the memory-mapped contents are being delivered, - // resulting in an odd mix. - lock.Reset(); + lock2.Reset(); // Read the file contents into the newly allocated memory. if ( !(flags & MAP_ANONYMOUS) ) { + ioctx_t kctx; SetupKernelIOCtx(&kctx); for ( size_t so_far = 0; so_far < aligned_size; ) { uint8_t* ptr = (uint8_t*) (new_segment.addr + so_far); size_t left = aligned_size - so_far; off_t pos = offset + so_far; - ssize_t num_bytes = desc->pread(&ctx, ptr, left, pos); + ssize_t num_bytes = desc->pread(&kctx, ptr, left, pos); if ( num_bytes < 0 ) { // TODO: How should this situation be handled? For now we'll @@ -383,6 +385,8 @@ void* sys_mmap(void* addr_ptr, size_t size, int prot, int flags, int fd, } } + lock1.Reset(); + return (void*) new_segment.addr; } @@ -400,7 +404,8 @@ int sys_mprotect(const void* addr_ptr, size_t size, int prot) prot |= PROT_KREAD | PROT_KWRITE | PROT_FORK; Process* process = CurrentProcess(); - ScopedLock lock(&process->segment_lock); + ScopedLock lock1(&process->segment_write_lock); + ScopedLock lock2(&process->segment_lock); if ( !Memory::ProtectMemory(process, addr, size, prot) ) return -1; @@ -421,7 +426,8 @@ int sys_munmap(void* addr_ptr, size_t size) size = Page::AlignUp(size); Process* process = CurrentProcess(); - ScopedLock lock(&process->segment_lock); + ScopedLock lock1(&process->segment_write_lock); + ScopedLock lock2(&process->segment_lock); Memory::UnmapMemory(process, addr, size); diff --git a/kernel/process.cpp b/kernel/process.cpp index 94af131c..75241717 100644 --- a/kernel/process.cpp +++ b/kernel/process.cpp @@ -149,6 +149,7 @@ Process::Process() segments = NULL; segments_used = 0; segments_length = 0; + segment_write_lock = KTHREAD_MUTEX_INITIALIZER; segment_lock = KTHREAD_MUTEX_INITIALIZER; user_timers_lock = KTHREAD_MUTEX_INITIALIZER; @@ -376,7 +377,8 @@ void Process::LastPrayer() void Process::ResetAddressSpace() { - ScopedLock lock(&segment_lock); + ScopedLock lock1(&segment_write_lock); + ScopedLock lock2(&segment_lock); assert(Memory::GetAddressSpace() == addrspace); @@ -798,6 +800,7 @@ void Process::ResetForExecute() bool Process::MapSegment(struct segment* result, void* hint, size_t size, int flags, int prot) { + // process->segment_write_lock is held at this point. // process->segment_lock is held at this point. if ( !size ) @@ -908,6 +911,7 @@ int Process::Execute(const char* programname, const uint8_t* program, struct segment tls_segment; struct segment auxcode_segment; + kthread_mutex_lock(&segment_write_lock); kthread_mutex_lock(&segment_lock); if ( !(MapSegment(&arg_segment, stack_hint, arg_size, 0, stack_prot) && @@ -917,11 +921,13 @@ int Process::Execute(const char* programname, const uint8_t* program, MapSegment(&auxcode_segment, auxcode_hint, auxcode_size, 0, auxcode_prot)) ) { kthread_mutex_unlock(&segment_lock); + kthread_mutex_unlock(&segment_write_lock); ResetForExecute(); return errno = ENOMEM, -1; } kthread_mutex_unlock(&segment_lock); + kthread_mutex_unlock(&segment_write_lock); char** target_argv = (char**) (arg_segment.addr + 0); char** target_envp = (char**) (arg_segment.addr + argv_size);