From 30b2c0dc85037c86d10ee14b46e4edfd42e785ec Mon Sep 17 00:00:00 2001 From: Brian Gianforcaro Date: Fri, 7 Aug 2020 02:29:05 -0700 Subject: [PATCH] Kernel: Use Userspace for the getsockopt syscall and Socket interface The way getsockopt is implemented for socket types requires us to push down Userspace using into those interfaces. This change does so, and utilizes proper copy implementations instead of the kind of haphazard pointer dereferencing that was occurring there before. --- Kernel/API/Syscall.h | 4 ++-- Kernel/Net/IPv4Socket.cpp | 12 ++++++++--- Kernel/Net/IPv4Socket.h | 2 +- Kernel/Net/LocalSocket.cpp | 20 +++++++++++------- Kernel/Net/LocalSocket.h | 2 +- Kernel/Net/Socket.cpp | 42 ++++++++++++++++++++++++-------------- Kernel/Net/Socket.h | 2 +- Kernel/Process.h | 2 +- Kernel/Syscalls/socket.cpp | 26 ++++++++++++----------- 9 files changed, 69 insertions(+), 43 deletions(-) diff --git a/Kernel/API/Syscall.h b/Kernel/API/Syscall.h index 3f676921f81..7fae203cdc7 100644 --- a/Kernel/API/Syscall.h +++ b/Kernel/API/Syscall.h @@ -306,8 +306,8 @@ struct SC_getsockopt_params { int sockfd; int level; int option; - void* value; - socklen_t* value_size; + Userspace value; + Userspace value_size; }; struct SC_setsockopt_params { diff --git a/Kernel/Net/IPv4Socket.cpp b/Kernel/Net/IPv4Socket.cpp index 6ae59ebad17..5cc3b6e3b0d 100644 --- a/Kernel/Net/IPv4Socket.cpp +++ b/Kernel/Net/IPv4Socket.cpp @@ -451,16 +451,22 @@ KResult IPv4Socket::setsockopt(int level, int option, Userspace use } } -KResult IPv4Socket::getsockopt(FileDescription& description, int level, int option, void* value, socklen_t* value_size) +KResult IPv4Socket::getsockopt(FileDescription& description, int level, int option, Userspace value, Userspace value_size) { if (level != IPPROTO_IP) return Socket::getsockopt(description, level, option, value, value_size); + socklen_t size; + if (!Process::current()->validate_read_and_copy_typed(&size, value_size)) + return KResult(-EFAULT); + switch (option) { case IP_TTL: - if (*value_size < sizeof(int)) + if (size < sizeof(int)) return KResult(-EINVAL); - *(int*)value = m_ttl; + copy_to_user(static_ptr_cast(value), (int*)&m_ttl); + size = sizeof(int); + copy_to_user(value_size, &size); return KSuccess; default: return KResult(-ENOPROTOOPT); diff --git a/Kernel/Net/IPv4Socket.h b/Kernel/Net/IPv4Socket.h index 28b4c075008..7a94f6df8a4 100644 --- a/Kernel/Net/IPv4Socket.h +++ b/Kernel/Net/IPv4Socket.h @@ -61,7 +61,7 @@ public: virtual KResultOr sendto(FileDescription&, const void*, size_t, int, const sockaddr*, socklen_t) override; virtual KResultOr recvfrom(FileDescription&, void*, size_t, int flags, sockaddr*, socklen_t*) override; virtual KResult setsockopt(int level, int option, Userspace, socklen_t) override; - virtual KResult getsockopt(FileDescription&, int level, int option, void*, socklen_t*) override; + virtual KResult getsockopt(FileDescription&, int level, int option, Userspace, Userspace) override; virtual int ioctl(FileDescription&, unsigned request, FlatPtr arg) override; diff --git a/Kernel/Net/LocalSocket.cpp b/Kernel/Net/LocalSocket.cpp index 4e3b56deff1..77c7308718c 100644 --- a/Kernel/Net/LocalSocket.cpp +++ b/Kernel/Net/LocalSocket.cpp @@ -344,24 +344,30 @@ String LocalSocket::absolute_path(const FileDescription& description) const return builder.to_string(); } -KResult LocalSocket::getsockopt(FileDescription& description, int level, int option, void* value, socklen_t* value_size) +KResult LocalSocket::getsockopt(FileDescription& description, int level, int option, Userspace value, Userspace value_size) { if (level != SOL_SOCKET) return Socket::getsockopt(description, level, option, value, value_size); + + socklen_t size; + if (!Process::current()->validate_read_and_copy_typed(&size, value_size)) + return KResult(-EFAULT); + switch (option) { case SO_PEERCRED: { - if (*value_size < sizeof(ucred)) + if (size < sizeof(ucred)) return KResult(-EINVAL); - auto& creds = *(ucred*)value; switch (role(description)) { case Role::Accepted: - creds = m_origin; - *value_size = sizeof(ucred); + copy_to_user(static_ptr_cast(value), &m_origin); + size = sizeof(ucred); + copy_to_user(value_size, &size); return KSuccess; case Role::Connected: - creds = m_acceptor; - *value_size = sizeof(ucred); + copy_to_user(static_ptr_cast(value), &m_acceptor); + size = sizeof(ucred); + copy_to_user(value_size, &size); return KSuccess; case Role::Connecting: return KResult(-ENOTCONN); diff --git a/Kernel/Net/LocalSocket.h b/Kernel/Net/LocalSocket.h index f01ce9bafae..60f5e2a52d2 100644 --- a/Kernel/Net/LocalSocket.h +++ b/Kernel/Net/LocalSocket.h @@ -62,7 +62,7 @@ public: virtual bool can_write(const FileDescription&, size_t) const override; virtual KResultOr sendto(FileDescription&, const void*, size_t, int, const sockaddr*, socklen_t) override; virtual KResultOr recvfrom(FileDescription&, void*, size_t, int flags, sockaddr*, socklen_t*) override; - virtual KResult getsockopt(FileDescription&, int level, int option, void*, socklen_t*) override; + virtual KResult getsockopt(FileDescription&, int level, int option, Userspace, Userspace) override; virtual KResult chown(FileDescription&, uid_t, gid_t) override; virtual KResult chmod(FileDescription&, mode_t) override; diff --git a/Kernel/Net/Socket.cpp b/Kernel/Net/Socket.cpp index 1c303373fc2..a021b806581 100644 --- a/Kernel/Net/Socket.cpp +++ b/Kernel/Net/Socket.cpp @@ -137,40 +137,52 @@ KResult Socket::setsockopt(int level, int option, Userspace user_va } } -KResult Socket::getsockopt(FileDescription&, int level, int option, void* value, socklen_t* value_size) +KResult Socket::getsockopt(FileDescription&, int level, int option, Userspace value, Userspace value_size) { + socklen_t size; + if (!Process::current()->validate_read_and_copy_typed(&size, value_size)) + return KResult(-EFAULT); + ASSERT(level == SOL_SOCKET); switch (option) { case SO_SNDTIMEO: - if (*value_size < sizeof(timeval)) + if (size < sizeof(timeval)) return KResult(-EINVAL); - *(timeval*)value = m_send_timeout; - *value_size = sizeof(timeval); + copy_to_user(static_ptr_cast(value), &m_send_timeout); + size = sizeof(timeval); + copy_to_user(value_size, &size); return KSuccess; case SO_RCVTIMEO: - if (*value_size < sizeof(timeval)) + if (size < sizeof(timeval)) return KResult(-EINVAL); - *(timeval*)value = m_receive_timeout; - *value_size = sizeof(timeval); + copy_to_user(static_ptr_cast(value), &m_receive_timeout); + size = sizeof(timeval); + copy_to_user(value_size, &size); return KSuccess; - case SO_ERROR: - if (*value_size < sizeof(int)) + case SO_ERROR: { + if (size < sizeof(int)) return KResult(-EINVAL); dbg() << "getsockopt(SO_ERROR): FIXME!"; - *(int*)value = 0; - *value_size = sizeof(int); + int errno = 0; + copy_to_user(static_ptr_cast(value), &errno); + size = sizeof(int); + copy_to_user(value_size, &size); return KSuccess; + } case SO_BINDTODEVICE: - if (*value_size < IFNAMSIZ) + if (size < IFNAMSIZ) return KResult(-EINVAL); if (m_bound_interface) { const auto& name = m_bound_interface->name(); auto length = name.length() + 1; - memcpy(value, name.characters(), length); - *value_size = length; + copy_to_user(static_ptr_cast(value), name.characters(), length); + size = length; + copy_to_user(value_size, &size); return KSuccess; } else { - *value_size = 0; + size = 0; + copy_to_user(value_size, &size); + return KResult(-EFAULT); } default: diff --git a/Kernel/Net/Socket.h b/Kernel/Net/Socket.h index f5481dac7f7..810310ed1a9 100644 --- a/Kernel/Net/Socket.h +++ b/Kernel/Net/Socket.h @@ -111,7 +111,7 @@ public: virtual KResultOr recvfrom(FileDescription&, void*, size_t, int flags, sockaddr*, socklen_t*) = 0; virtual KResult setsockopt(int level, int option, Userspace, socklen_t); - virtual KResult getsockopt(FileDescription&, int level, int option, void*, socklen_t*); + virtual KResult getsockopt(FileDescription&, int level, int option, Userspace, Userspace); pid_t origin_pid() const { return m_origin.pid; } uid_t origin_uid() const { return m_origin.uid; } diff --git a/Kernel/Process.h b/Kernel/Process.h index b086926a8d4..b983142b066 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -295,7 +295,7 @@ public: int sys$shutdown(int sockfd, int how); ssize_t sys$sendto(const Syscall::SC_sendto_params*); ssize_t sys$recvfrom(const Syscall::SC_recvfrom_params*); - int sys$getsockopt(const Syscall::SC_getsockopt_params*); + int sys$getsockopt(Userspace); int sys$setsockopt(Userspace); int sys$getsockname(Userspace); int sys$getpeername(Userspace); diff --git a/Kernel/Syscalls/socket.cpp b/Kernel/Syscalls/socket.cpp index 234fe56e164..0793be86694 100644 --- a/Kernel/Syscalls/socket.cpp +++ b/Kernel/Syscalls/socket.cpp @@ -316,22 +316,24 @@ int Process::sys$getpeername(Userspace us return get_sock_or_peer_name(params); } -int Process::sys$getsockopt(const Syscall::SC_getsockopt_params* params) +int Process::sys$getsockopt(Userspace user_params) { - if (!validate_read_typed(params)) + Syscall::SC_getsockopt_params params; + if (!validate_read_and_copy_typed(¶ms, user_params)) return -EFAULT; - SmapDisabler disabler; + int sockfd = params.sockfd; + int level = params.level; + int option = params.option; + Userspace user_value = params.value; + Userspace user_value_size = params.value_size; - int sockfd = params->sockfd; - int level = params->level; - int option = params->option; - void* value = params->value; - socklen_t* value_size = params->value_size; - - if (!validate_write_typed(value_size)) + if (!validate_write_typed(user_value_size)) return -EFAULT; - if (!validate_write(value, *value_size)) + socklen_t value_size; + if (!validate_read_and_copy_typed(&value_size, user_value_size)) + return -EFAULT; + if (!validate_write(user_value, value_size)) return -EFAULT; auto description = file_description(sockfd); if (!description) @@ -345,7 +347,7 @@ int Process::sys$getsockopt(const Syscall::SC_getsockopt_params* params) } else { REQUIRE_PROMISE_FOR_SOCKET_DOMAIN(socket.domain()); } - return socket.getsockopt(*description, level, option, value, value_size); + return socket.getsockopt(*description, level, option, user_value, user_value_size); } int Process::sys$setsockopt(Userspace user_params)