From 952bb95baa36d385fa26ea40777dbe2addb1db65 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 10 Jan 2020 12:20:36 +0100 Subject: [PATCH] Kernel: Enable SMAP protection during the execve() syscall The userspace execve() wrapper now measures all the strings and puts them in a neat and tidy structure on the stack. This way we know exactly how much to copy in the kernel, and we don't have to use the SMAP-violating validate_read_str(). :^) --- Kernel/Process.cpp | 71 +++++++++++++++++++-------------------- Kernel/Process.h | 2 +- Kernel/Syscall.h | 16 +++++++++ Libraries/LibC/unistd.cpp | 27 ++++++++++++++- 4 files changed, 78 insertions(+), 38 deletions(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index c9b0cae3be..ed31b2aab7 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -909,50 +909,49 @@ int Process::exec(String path, Vector arguments, Vector environm return 0; } -int Process::sys$execve(const char* filename, const char** argv, const char** envp) +int Process::sys$execve(const Syscall::SC_execve_params* user_params) { - SmapDisabler disabler; // NOTE: Be extremely careful with allocating any kernel memory in exec(). // On success, the kernel stack will be lost. - if (!validate_read_str(filename)) + Syscall::SC_execve_params params; + if (!validate_read_typed(user_params)) return -EFAULT; - if (!*filename) + copy_from_user(¶ms, user_params, sizeof(params)); + + if (params.arguments.length > ARG_MAX || params.environment.length > ARG_MAX) + return -E2BIG; + + if (!validate_read(params.path.characters, params.path.length)) + return -EFAULT; + + if (params.path.length == 0) return -ENOENT; - if (argv) { - if (!validate_read_typed(argv)) - return -EFAULT; - for (size_t i = 0; argv[i]; ++i) { - if (!validate_read_str(argv[i])) - return -EFAULT; - } - } - if (envp) { - if (!validate_read_typed(envp)) - return -EFAULT; - for (size_t i = 0; envp[i]; ++i) { - if (!validate_read_str(envp[i])) - return -EFAULT; - } - } - String path(filename); + auto copy_user_strings = [&](const auto& list, auto& output) { + if (!list.length) + return true; + if (!validate_read_typed(list.strings, list.length)) + return false; + Vector strings; + strings.resize(list.length); + copy_from_user(strings.data(), list.strings, list.length * sizeof(Syscall::SyscallString)); + for (size_t i = 0; i < list.length; ++i) { + if (!validate_read(strings[i].characters, strings[i].length)) + return false; + output.append(copy_string_from_user(strings[i].characters, strings[i].length)); + } + return true; + }; + Vector arguments; - Vector environment; - { - auto parts = path.split('/'); - if (argv) { - for (size_t i = 0; argv[i]; ++i) { - arguments.append(argv[i]); - } - } else { - arguments.append(parts.last()); - } + if (!copy_user_strings(params.arguments, arguments)) + return -EFAULT; - if (envp) { - for (size_t i = 0; envp[i]; ++i) - environment.append(envp[i]); - } - } + Vector environment; + if (!copy_user_strings(params.environment, environment)) + return -EFAULT; + + auto path = copy_string_from_user(params.path.characters, params.path.length); int rc = exec(move(path), move(arguments), move(environment)); ASSERT(rc < 0); // We should never continue after a successful exec! diff --git a/Kernel/Process.h b/Kernel/Process.h index 9f937665d0..5c9980b557 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -155,7 +155,7 @@ public: int sys$ttyname_r(int fd, char*, ssize_t); int sys$ptsname_r(int fd, char*, ssize_t); pid_t sys$fork(RegisterDump&); - int sys$execve(const char* filename, const char** argv, const char** envp); + int sys$execve(const Syscall::SC_execve_params*); int sys$getdtablesize(); int sys$dup(int oldfd); int sys$dup2(int oldfd, int newfd); diff --git a/Kernel/Syscall.h b/Kernel/Syscall.h index 6a7b49d7c8..8c973d3660 100644 --- a/Kernel/Syscall.h +++ b/Kernel/Syscall.h @@ -300,6 +300,22 @@ struct SC_set_mmap_name_params { size_t name_length; }; +struct SyscallString { + const char* characters { nullptr }; + size_t length { 0 }; +}; + +struct SyscallStringList { + SyscallString* strings { nullptr }; + size_t length { 0 }; +}; + +struct SC_execve_params { + SyscallString path; + SyscallStringList arguments; + SyscallStringList environment; +}; + void initialize(); int sync(); diff --git a/Libraries/LibC/unistd.cpp b/Libraries/LibC/unistd.cpp index 13252e7166..2f4debf7b3 100644 --- a/Libraries/LibC/unistd.cpp +++ b/Libraries/LibC/unistd.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include #include @@ -49,7 +50,31 @@ int execv(const char* path, char* const argv[]) int execve(const char* filename, char* const argv[], char* const envp[]) { - int rc = syscall(SC_execve, filename, argv, envp); + size_t arg_count = 0; + for (size_t i = 0; argv[i]; ++i) + ++arg_count; + + size_t env_count = 0; + for (size_t i = 0; envp[i]; ++i) + ++env_count; + + auto copy_strings = [&](auto& vec, size_t count, auto& output) { + output.length = count; + for (size_t i = 0; vec[i]; ++i) { + output.strings[i].characters = vec[i]; + output.strings[i].length = strlen(vec[i]); + } + }; + + Syscall::SC_execve_params params; + params.arguments.strings = (Syscall::SyscallString*)alloca(arg_count * sizeof(Syscall::SyscallString)); + params.environment.strings = (Syscall::SyscallString*)alloca(env_count * sizeof(Syscall::SyscallString)); + + params.path = { filename, strlen(filename) }; + copy_strings(argv, arg_count, params.arguments); + copy_strings(envp, env_count, params.environment); + + int rc = syscall(SC_execve, ¶ms); __RETURN_WITH_ERRNO(rc, rc, -1); }