From 6081c7651582065c36d5316ae526140b2a4adc46 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Tue, 21 Jan 2020 13:14:26 +0100 Subject: [PATCH] Kernel: Make O_RDONLY non-zero Sergey suggested that having a non-zero O_RDONLY would make some things less confusing, and it seems like he's right about that. We can now easily check read/write permissions separately instead of dancing around with the bits. This patch also fixes unveil() validation for O_RDWR which previously forgot to check for "r" permission. --- Kernel/FileSystem/FileDescription.h | 12 ++--------- Kernel/FileSystem/VirtualFileSystem.cpp | 27 ++++++++++++------------- Kernel/FileSystem/VirtualFileSystem.h | 6 +++--- Kernel/Net/LocalSocket.cpp | 2 +- Kernel/Process.cpp | 4 ++-- Libraries/LibC/fcntl.h | 6 +++--- 6 files changed, 24 insertions(+), 33 deletions(-) diff --git a/Kernel/FileSystem/FileDescription.h b/Kernel/FileSystem/FileDescription.h index e907545d9b..4638f00028 100644 --- a/Kernel/FileSystem/FileDescription.h +++ b/Kernel/FileSystem/FileDescription.h @@ -59,16 +59,8 @@ public: void set_rw_mode(int options) { - if (options & O_WRONLY) { - set_readable(false); - set_writable(true); - } else if (options & O_RDWR) { - set_readable(true); - set_writable(true); - } else { - set_readable(true); - set_writable(false); - } + set_readable(options & O_RDONLY); + set_writable(options & O_WRONLY); } int close(); diff --git a/Kernel/FileSystem/VirtualFileSystem.cpp b/Kernel/FileSystem/VirtualFileSystem.cpp index d87e70c28b..6b4ba042eb 100644 --- a/Kernel/FileSystem/VirtualFileSystem.cpp +++ b/Kernel/FileSystem/VirtualFileSystem.cpp @@ -239,13 +239,10 @@ KResultOr> VFS::open(StringView path, int options bool should_truncate_file = false; - // NOTE: Read permission is a bit weird, since O_RDONLY == 0, - // so we check if (NOT write_only OR read_and_write) - if (!(options & O_WRONLY) || (options & O_RDWR)) { - if (!metadata.may_read(current->process())) - return KResult(-EACCES); - } - if ((options & O_WRONLY) || (options & O_RDWR)) { + if ((options & O_RDONLY) && !metadata.may_read(current->process())) + return KResult(-EACCES); + + if (options & O_WRONLY) { if (!metadata.may_write(current->process())) return KResult(-EACCES); if (metadata.is_directory()) @@ -748,21 +745,23 @@ KResult VFS::validate_path_against_process_veil(StringView path, int options) } return KSuccess; } - if ((options & O_RDWR) || (options & O_WRONLY)) { + if (options & O_RDONLY) { + if (!(unveiled_path->permissions & UnveiledPath::Access::Read)) { + dbg() << *current << " rejecting path '" << path << "' since it hasn't been unveiled with 'r' permission."; + return KResult(-EACCES); + } + } + if (options & O_WRONLY) { if (!(unveiled_path->permissions & UnveiledPath::Access::Write)) { dbg() << *current << " rejecting path '" << path << "' since it hasn't been unveiled with 'w' permission."; return KResult(-EACCES); } - } else if (options & O_EXEC) { + } + if (options & O_EXEC) { if (!(unveiled_path->permissions & UnveiledPath::Access::Execute)) { dbg() << *current << " rejecting path '" << path << "' since it hasn't been unveiled with 'x' permission."; return KResult(-EACCES); } - } else { - if (!(unveiled_path->permissions & UnveiledPath::Access::Read)) { - dbg() << *current << " rejecting path '" << path << "' since it hasn't been unveiled with 'r' permission."; - return KResult(-EACCES); - } } return KSuccess; } diff --git a/Kernel/FileSystem/VirtualFileSystem.h b/Kernel/FileSystem/VirtualFileSystem.h index 27fcc172dc..3984f9eb3a 100644 --- a/Kernel/FileSystem/VirtualFileSystem.h +++ b/Kernel/FileSystem/VirtualFileSystem.h @@ -38,9 +38,9 @@ #include #include -#define O_RDONLY 0 -#define O_WRONLY 1 -#define O_RDWR 2 +#define O_RDONLY 1 +#define O_WRONLY 2 +#define O_RDWR 3 #define O_EXEC 4 #define O_CREAT 0100 #define O_EXCL 0200 diff --git a/Kernel/Net/LocalSocket.cpp b/Kernel/Net/LocalSocket.cpp index 12269b83fa..0c59ba14f8 100644 --- a/Kernel/Net/LocalSocket.cpp +++ b/Kernel/Net/LocalSocket.cpp @@ -111,7 +111,7 @@ KResult LocalSocket::bind(const sockaddr* user_address, socklen_t address_size) mode_t mode = S_IFSOCK | (m_prebind_mode & 04777); UidAndGid owner { m_prebind_uid, m_prebind_gid }; - auto result = VFS::the().open(path, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW_NOERROR, mode, current->process().current_directory(), owner); + auto result = VFS::the().open(path, O_CREAT | O_EXCL | O_NOFOLLOW_NOERROR, mode, current->process().current_directory(), owner); if (result.is_error()) { if (result.error() == -EEXIST) return KResult(-EADDRINUSE); diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 6be807edaa..56dc3ea059 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -1945,9 +1945,9 @@ int Process::sys$open(const Syscall::SC_open_params* user_params) if (options & O_UNLINK_INTERNAL) return -EINVAL; - if ((options & O_RDWR) || (options & O_WRONLY)) + if (options & O_WRONLY) REQUIRE_PROMISE(wpath); - else + else if (options & O_RDONLY) REQUIRE_PROMISE(rpath); if (options & O_CREAT) diff --git a/Libraries/LibC/fcntl.h b/Libraries/LibC/fcntl.h index b331da9b16..1fa8f54519 100644 --- a/Libraries/LibC/fcntl.h +++ b/Libraries/LibC/fcntl.h @@ -39,9 +39,9 @@ __BEGIN_DECLS #define FD_CLOEXEC 1 -#define O_RDONLY 0 -#define O_WRONLY 1 -#define O_RDWR 2 +#define O_RDONLY 1 +#define O_WRONLY 2 +#define O_RDWR 3 #define O_ACCMODE 3 #define O_EXEC 4 #define O_CREAT 0100