From 4b2651ddab2c72106834277038dc91ffb26cd727 Mon Sep 17 00:00:00 2001 From: Brian Gianforcaro Date: Tue, 27 Jul 2021 23:59:24 -0700 Subject: [PATCH] Kernel: Track allocated FileDescriptionAndFlag elements in each Process The way the Process::FileDescriptions::allocate() API works today means that two callers who allocate back to back without associating a FileDescription with the allocated FD, will receive the same FD and thus one will stomp over the other. Naively tracking which FileDescriptions are allocated and moving onto the next would introduce other bugs however, as now if you "allocate" a fd and then return early further down the control flow of the syscall you would leak that fd. This change modifies this behavior by tracking which descriptions are allocated and then having an RAII type to "deallocate" the fd if the association is not setup the end of it's scope. --- Kernel/Process.cpp | 23 ++++++++++----- Kernel/Process.h | 48 ++++++++++++++++++++++++++++++- Kernel/Syscalls/anon_create.cpp | 6 ++-- Kernel/Syscalls/dup2.cpp | 2 ++ Kernel/Syscalls/execve.cpp | 7 +++-- Kernel/Syscalls/fcntl.cpp | 6 ++-- Kernel/Syscalls/inode_watcher.cpp | 12 ++++---- Kernel/Syscalls/open.cpp | 6 ++-- Kernel/Syscalls/pipe.cpp | 17 +++++------ Kernel/Syscalls/sendfd.cpp | 6 ++-- Kernel/Syscalls/socket.cpp | 16 +++++------ 11 files changed, 104 insertions(+), 45 deletions(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 4ce3f826af..8e49c084ed 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -169,9 +169,14 @@ RefPtr Process::create_user_process(RefPtr& first_thread, const } auto& device_to_use_as_tty = tty ? (CharacterDevice&)*tty : NullDevice::the(); auto description = device_to_use_as_tty.open(O_RDWR).value(); - process->m_fds[0].set(*description); - process->m_fds[1].set(*description); - process->m_fds[2].set(*description); + + auto setup_description = [&process, &description](int fd) { + process->m_fds.m_fds_metadatas[fd].allocate(); + process->m_fds[fd].set(*description); + }; + setup_description(0); + setup_description(1); + setup_description(2); error = process->exec(path, move(arguments), move(environment)); if (error != 0) { @@ -411,11 +416,13 @@ RefPtr Process::from_pid(ProcessID pid) const Process::FileDescriptionAndFlags& Process::FileDescriptions::at(size_t i) const { ScopedSpinLock lock(m_fds_lock); + VERIFY(m_fds_metadatas[i].is_allocated()); return m_fds_metadatas[i]; } Process::FileDescriptionAndFlags& Process::FileDescriptions::at(size_t i) { ScopedSpinLock lock(m_fds_lock); + VERIFY(m_fds_metadatas[i].is_allocated()); return m_fds_metadatas[i]; } @@ -465,14 +472,16 @@ size_t Process::FileDescriptions::open_count() const return count; } -KResultOr Process::FileDescriptions::allocate(int first_candidate_fd) +KResultOr Process::FileDescriptions::allocate(int first_candidate_fd) { ScopedSpinLock lock(m_fds_lock); for (size_t i = first_candidate_fd; i < max_open(); ++i) { - if (!m_fds_metadatas[i]) - return i; + if (!m_fds_metadatas[i].is_allocated()) { + m_fds_metadatas[i].allocate(); + return Process::ScopedDescriptionAllocation { static_cast(i), &m_fds_metadatas[i] }; + } } - return KResult(EMFILE); + return EMFILE; } Time kgettimeofday() diff --git a/Kernel/Process.h b/Kernel/Process.h index ec8fa927a3..02f77a3bd6 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -597,6 +597,19 @@ public: operator bool() const { return !!m_description; } bool is_valid() const { return !m_description.is_null(); } + bool is_allocated() const { return m_is_allocated; } + void allocate() + { + VERIFY(!m_is_allocated); + VERIFY(!is_valid()); + m_is_allocated = true; + } + void deallocate() + { + VERIFY(m_is_allocated); + VERIFY(!is_valid()); + m_is_allocated = false; + } FileDescription* description() { return m_description; } const FileDescription* description() const { return m_description; } @@ -610,6 +623,7 @@ public: private: RefPtr m_description; + bool m_is_allocated { false }; u32 m_flags { 0 }; // Note: This is needed so when we generate inodes for ProcFS, we know that @@ -617,6 +631,7 @@ public: InodeIndex m_global_procfs_inode_index; }; + class ScopedDescriptionAllocation; class FileDescriptions { friend class Process; @@ -641,7 +656,7 @@ public: void enumerate(Function) const; void change_each(Function); - KResultOr allocate(int first_candidate_fd = 0); + KResultOr allocate(int first_candidate_fd = 0); size_t open_count() const; bool try_resize(size_t size) { return m_fds_metadatas.try_resize(size); } @@ -668,6 +683,37 @@ public: Vector m_fds_metadatas; }; + class ScopedDescriptionAllocation { + AK_MAKE_NONCOPYABLE(ScopedDescriptionAllocation); + + public: + ScopedDescriptionAllocation() = default; + ScopedDescriptionAllocation(int tracked_fd, FileDescriptionAndFlags* description) + : fd(tracked_fd) + , m_description(description) + { + } + + ScopedDescriptionAllocation(ScopedDescriptionAllocation&& other) + : fd(other.fd) + { + // Take over the responsibility of tracking to deallocation. + swap(m_description, other.m_description); + } + + ~ScopedDescriptionAllocation() + { + if (m_description && m_description->is_allocated() && !m_description->is_valid()) { + m_description->deallocate(); + } + } + + const int fd { -1 }; + + private: + FileDescriptionAndFlags* m_description { nullptr }; + }; + FileDescriptions& fds() { return m_fds; } const FileDescriptions& fds() const { return m_fds; } diff --git a/Kernel/Syscalls/anon_create.cpp b/Kernel/Syscalls/anon_create.cpp index e20171ce09..67073993ff 100644 --- a/Kernel/Syscalls/anon_create.cpp +++ b/Kernel/Syscalls/anon_create.cpp @@ -28,7 +28,7 @@ KResultOr Process::sys$anon_create(size_t size, int options) auto new_fd_or_error = m_fds.allocate(); if (new_fd_or_error.is_error()) return new_fd_or_error.error(); - auto new_fd = new_fd_or_error.value(); + auto new_fd = new_fd_or_error.release_value(); auto vmobject = AnonymousVMObject::try_create_purgeable_with_size(size, AllocationStrategy::Reserve); if (!vmobject) return ENOMEM; @@ -48,8 +48,8 @@ KResultOr Process::sys$anon_create(size_t size, int options) if (options & O_CLOEXEC) fd_flags |= FD_CLOEXEC; - m_fds[new_fd].set(move(description), fd_flags); - return new_fd; + m_fds[new_fd.fd].set(move(description), fd_flags); + return new_fd.fd; } } diff --git a/Kernel/Syscalls/dup2.cpp b/Kernel/Syscalls/dup2.cpp index a9946e0f04..b836f1282e 100644 --- a/Kernel/Syscalls/dup2.cpp +++ b/Kernel/Syscalls/dup2.cpp @@ -20,6 +20,8 @@ KResultOr Process::sys$dup2(int old_fd, int new_fd) return new_fd; if (new_fd < 0 || static_cast(new_fd) >= fds().max_open()) return EINVAL; + if (!m_fds.m_fds_metadatas[new_fd].is_allocated()) + m_fds.m_fds_metadatas[new_fd].allocate(); m_fds[new_fd].set(*description); return new_fd; } diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index 090551998d..e96038e8f4 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -591,12 +591,13 @@ KResult Process::do_exec(NonnullRefPtr main_program_description int main_program_fd = -1; if (interpreter_description) { - main_program_fd = m_fds.allocate().value(); - VERIFY(main_program_fd >= 0); + auto main_program_fd_wrapper = m_fds.allocate().release_value(); + VERIFY(main_program_fd_wrapper.fd >= 0); auto seek_result = main_program_description->seek(0, SEEK_SET); VERIFY(!seek_result.is_error()); main_program_description->set_readable(true); - m_fds[main_program_fd].set(move(main_program_description), FD_CLOEXEC); + m_fds[main_program_fd_wrapper.fd].set(move(main_program_description), FD_CLOEXEC); + main_program_fd = main_program_fd_wrapper.fd; } new_main_thread = nullptr; diff --git a/Kernel/Syscalls/fcntl.cpp b/Kernel/Syscalls/fcntl.cpp index f6871913f0..61a9a6ef99 100644 --- a/Kernel/Syscalls/fcntl.cpp +++ b/Kernel/Syscalls/fcntl.cpp @@ -28,9 +28,9 @@ KResultOr Process::sys$fcntl(int fd, int cmd, u32 arg) auto new_fd_or_error = fds().allocate(arg_fd); if (new_fd_or_error.is_error()) return new_fd_or_error.error(); - auto new_fd = new_fd_or_error.value(); - m_fds[new_fd].set(*description); - return new_fd; + auto new_fd = new_fd_or_error.release_value(); + m_fds[new_fd.fd].set(*description); + return new_fd.fd; } case F_GETFD: return m_fds[fd].flags(); diff --git a/Kernel/Syscalls/inode_watcher.cpp b/Kernel/Syscalls/inode_watcher.cpp index ae09d51c42..0d94f99b92 100644 --- a/Kernel/Syscalls/inode_watcher.cpp +++ b/Kernel/Syscalls/inode_watcher.cpp @@ -21,7 +21,7 @@ KResultOr Process::sys$create_inode_watcher(u32 flags) auto fd_or_error = m_fds.allocate(); if (fd_or_error.is_error()) return fd_or_error.error(); - auto fd = fd_or_error.value(); + auto inode_watcher_fd = fd_or_error.release_value(); auto watcher_or_error = InodeWatcher::create(); if (watcher_or_error.is_error()) @@ -31,15 +31,15 @@ KResultOr Process::sys$create_inode_watcher(u32 flags) if (description_or_error.is_error()) return description_or_error.error(); - m_fds[fd].set(description_or_error.release_value()); - m_fds[fd].description()->set_readable(true); + m_fds[inode_watcher_fd.fd].set(description_or_error.release_value()); + m_fds[inode_watcher_fd.fd].description()->set_readable(true); if (flags & static_cast(InodeWatcherFlags::Nonblock)) - m_fds[fd].description()->set_blocking(false); + m_fds[inode_watcher_fd.fd].description()->set_blocking(false); if (flags & static_cast(InodeWatcherFlags::CloseOnExec)) - m_fds[fd].set_flags(m_fds[fd].flags() | FD_CLOEXEC); + m_fds[inode_watcher_fd.fd].set_flags(m_fds[inode_watcher_fd.fd].flags() | FD_CLOEXEC); - return fd; + return inode_watcher_fd.fd; } KResultOr Process::sys$inode_watcher_add_watch(Userspace user_params) diff --git a/Kernel/Syscalls/open.cpp b/Kernel/Syscalls/open.cpp index 71d8c016cb..43b961a82a 100644 --- a/Kernel/Syscalls/open.cpp +++ b/Kernel/Syscalls/open.cpp @@ -49,7 +49,7 @@ KResultOr Process::sys$open(Userspace u auto fd_or_error = m_fds.allocate(); if (fd_or_error.is_error()) return fd_or_error.error(); - auto fd = fd_or_error.value(); + auto new_fd = fd_or_error.release_value(); RefPtr base; if (dirfd == AT_FDCWD) { base = current_directory(); @@ -73,8 +73,8 @@ KResultOr Process::sys$open(Userspace u return ENXIO; u32 fd_flags = (options & O_CLOEXEC) ? FD_CLOEXEC : 0; - m_fds[fd].set(move(description), fd_flags); - return fd; + m_fds[new_fd.fd].set(move(description), fd_flags); + return new_fd.fd; } KResultOr Process::sys$close(int fd) diff --git a/Kernel/Syscalls/pipe.cpp b/Kernel/Syscalls/pipe.cpp index d2bd73e5f2..93bbd31801 100644 --- a/Kernel/Syscalls/pipe.cpp +++ b/Kernel/Syscalls/pipe.cpp @@ -33,19 +33,20 @@ KResultOr Process::sys$pipe(int pipefd[2], int flags) auto reader_fd_or_error = m_fds.allocate(); if (reader_fd_or_error.is_error()) return reader_fd_or_error.error(); - auto reader_fd = reader_fd_or_error.value(); - m_fds[reader_fd].set(open_reader_result.release_value(), fd_flags); - m_fds[reader_fd].description()->set_readable(true); - if (!copy_to_user(&pipefd[0], &reader_fd)) + auto reader_fd = reader_fd_or_error.release_value(); + m_fds[reader_fd.fd].set(open_reader_result.release_value(), fd_flags); + m_fds[reader_fd.fd].description()->set_readable(true); + if (!copy_to_user(&pipefd[0], &reader_fd.fd)) return EFAULT; auto writer_fd_or_error = m_fds.allocate(); if (writer_fd_or_error.is_error()) return writer_fd_or_error.error(); - auto writer_fd = writer_fd_or_error.value(); - m_fds[writer_fd].set(open_writer_result.release_value(), fd_flags); - m_fds[writer_fd].description()->set_writable(true); - if (!copy_to_user(&pipefd[1], &writer_fd)) + auto writer_fd = writer_fd_or_error.release_value(); + m_fds[writer_fd.fd].set(open_writer_result.release_value(), fd_flags); + m_fds[writer_fd.fd].description()->set_writable(true); + + if (!copy_to_user(&pipefd[1], &writer_fd.fd)) return EFAULT; return 0; diff --git a/Kernel/Syscalls/sendfd.cpp b/Kernel/Syscalls/sendfd.cpp index ba7e2c1d48..9981cbe52a 100644 --- a/Kernel/Syscalls/sendfd.cpp +++ b/Kernel/Syscalls/sendfd.cpp @@ -49,7 +49,7 @@ KResultOr Process::sys$recvfd(int sockfd, int options) auto new_fd_or_error = m_fds.allocate(); if (new_fd_or_error.is_error()) return new_fd_or_error.error(); - auto new_fd = new_fd_or_error.value(); + auto new_fd = new_fd_or_error.release_value(); auto& local_socket = static_cast(socket); auto received_descriptor_or_error = local_socket.recvfd(*socket_description); @@ -61,8 +61,8 @@ KResultOr Process::sys$recvfd(int sockfd, int options) if (options & O_CLOEXEC) fd_flags |= FD_CLOEXEC; - m_fds[new_fd].set(*received_descriptor_or_error.value(), fd_flags); - return new_fd; + m_fds[new_fd.fd].set(*received_descriptor_or_error.value(), fd_flags); + return new_fd.fd; } } diff --git a/Kernel/Syscalls/socket.cpp b/Kernel/Syscalls/socket.cpp index 87d75ce368..522881c224 100644 --- a/Kernel/Syscalls/socket.cpp +++ b/Kernel/Syscalls/socket.cpp @@ -42,15 +42,15 @@ KResultOr Process::sys$socket(int domain, int type, int protocol) auto fd_or_error = m_fds.allocate(); if (fd_or_error.is_error()) return fd_or_error.error(); - auto fd = fd_or_error.value(); + auto socket_fd = fd_or_error.release_value(); auto result = Socket::create(domain, type, protocol); if (result.is_error()) return result.error(); auto description_result = FileDescription::create(*result.value()); if (description_result.is_error()) return description_result.error(); - setup_socket_fd(fd, description_result.value(), type); - return fd; + setup_socket_fd(socket_fd.fd, description_result.value(), type); + return socket_fd.fd; } KResultOr Process::sys$bind(int sockfd, Userspace address, socklen_t address_length) @@ -104,7 +104,7 @@ KResultOr Process::sys$accept4(Userspace Process::sys$accept4(Userspaceset_setup_state(Socket::SetupState::Completed); - return accepted_socket_fd; + return accepted_socket_fd.fd; } KResultOr Process::sys$connect(int sockfd, Userspace user_address, socklen_t user_address_size) @@ -416,13 +416,13 @@ KResultOr Process::sys$socketpair(Userspace