From 292cd53192e019cbc3b860bfcad99972cff2a29b Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 31 Jul 2020 00:17:25 +0200 Subject: [PATCH] Kernel: Remove SmapDisabler in sys$ioctl() Use copy_{to,from}_user() in the various File::ioctl() implementations instead of disabling SMAP wholesale in sys$ioctl(). This patch does not port IPv4Socket::ioctl() to those API's since that will be more involved. That function now creates a local SmapDisabler. --- Kernel/Devices/BXVGADevice.cpp | 45 ++++++++++++++++++------------ Kernel/Devices/MBVGADevice.cpp | 30 ++++++++++++-------- Kernel/Net/IPv4Socket.cpp | 2 ++ Kernel/Syscalls/ioctl.cpp | 1 - Kernel/TTY/TTY.cpp | 51 +++++++++++++++++++++------------- 5 files changed, 78 insertions(+), 51 deletions(-) diff --git a/Kernel/Devices/BXVGADevice.cpp b/Kernel/Devices/BXVGADevice.cpp index b2c0107b79..1e3a4b5168 100644 --- a/Kernel/Devices/BXVGADevice.cpp +++ b/Kernel/Devices/BXVGADevice.cpp @@ -26,11 +26,11 @@ #include #include +#include #include #include #include #include -#include #include #include @@ -200,14 +200,16 @@ int BXVGADevice::ioctl(FileDescription&, unsigned request, FlatPtr arg) auto* out = (size_t*)arg; if (!Process::current()->validate_write_typed(out)) return -EFAULT; - *out = framebuffer_size_in_bytes(); + size_t value = framebuffer_size_in_bytes(); + copy_to_user(out, &value); return 0; } case FB_IOCTL_GET_BUFFER: { auto* index = (int*)arg; if (!Process::current()->validate_write_typed(index)) return -EFAULT; - *index = m_y_offset == 0 ? 0 : 1; + int value = m_y_offset == 0 ? 0 : 1; + copy_to_user(index, &value); return 0; } case FB_IOCTL_SET_BUFFER: { @@ -217,35 +219,42 @@ int BXVGADevice::ioctl(FileDescription&, unsigned request, FlatPtr arg) return 0; } case FB_IOCTL_GET_RESOLUTION: { - auto* resolution = (FBResolution*)arg; - if (!Process::current()->validate_write_typed(resolution)) + auto* user_resolution = (FBResolution*)arg; + if (!Process::current()->validate_write_typed(user_resolution)) return -EFAULT; - resolution->pitch = m_framebuffer_pitch; - resolution->width = m_framebuffer_width; - resolution->height = m_framebuffer_height; + FBResolution resolution; + resolution.pitch = m_framebuffer_pitch; + resolution.width = m_framebuffer_width; + resolution.height = m_framebuffer_height; + copy_to_user(user_resolution, &resolution); return 0; } case FB_IOCTL_SET_RESOLUTION: { - auto* resolution = (FBResolution*)arg; - if (!Process::current()->validate_read_typed(resolution) || !Process::current()->validate_write_typed(resolution)) + auto* user_resolution = (FBResolution*)arg; + if (!Process::current()->validate_write_typed(user_resolution)) return -EFAULT; - if (resolution->width > MAX_RESOLUTION_WIDTH || resolution->height > MAX_RESOLUTION_HEIGHT) + FBResolution resolution; + if (!Process::current()->validate_read_and_copy_typed(&resolution, user_resolution)) + return -EFAULT; + if (resolution.width > MAX_RESOLUTION_WIDTH || resolution.height > MAX_RESOLUTION_HEIGHT) return -EINVAL; - if (!set_resolution(resolution->width, resolution->height)) { + if (!set_resolution(resolution.width, resolution.height)) { #ifdef BXVGA_DEBUG dbg() << "Reverting Resolution: [" << m_framebuffer_width << "x" << m_framebuffer_height << "]"; #endif - resolution->pitch = m_framebuffer_pitch; - resolution->width = m_framebuffer_width; - resolution->height = m_framebuffer_height; + resolution.pitch = m_framebuffer_pitch; + resolution.width = m_framebuffer_width; + resolution.height = m_framebuffer_height; + copy_to_user(user_resolution, &resolution); return -EINVAL; } #ifdef BXVGA_DEBUG dbg() << "New resolution: [" << m_framebuffer_width << "x" << m_framebuffer_height << "]"; #endif - resolution->pitch = m_framebuffer_pitch; - resolution->width = m_framebuffer_width; - resolution->height = m_framebuffer_height; + resolution.pitch = m_framebuffer_pitch; + resolution.width = m_framebuffer_width; + resolution.height = m_framebuffer_height; + copy_to_user(user_resolution, &resolution); return 0; } default: diff --git a/Kernel/Devices/MBVGADevice.cpp b/Kernel/Devices/MBVGADevice.cpp index b13a9f4d91..f19de93e41 100644 --- a/Kernel/Devices/MBVGADevice.cpp +++ b/Kernel/Devices/MBVGADevice.cpp @@ -81,32 +81,38 @@ int MBVGADevice::ioctl(FileDescription&, unsigned request, FlatPtr arg) auto* out = (size_t*)arg; if (!Process::current()->validate_write_typed(out)) return -EFAULT; - *out = framebuffer_size_in_bytes(); + size_t value = framebuffer_size_in_bytes(); + copy_to_user(out, &value); return 0; } case FB_IOCTL_GET_BUFFER: { auto* index = (int*)arg; if (!Process::current()->validate_write_typed(index)) return -EFAULT; - *index = 0; + int value = 0; + copy_to_user(index, &value); return 0; } case FB_IOCTL_GET_RESOLUTION: { - auto* resolution = (FBResolution*)arg; - if (!Process::current()->validate_write_typed(resolution)) + auto* user_resolution = (FBResolution*)arg; + if (!Process::current()->validate_write_typed(user_resolution)) return -EFAULT; - resolution->pitch = m_framebuffer_pitch; - resolution->width = m_framebuffer_width; - resolution->height = m_framebuffer_height; + FBResolution resolution; + resolution.pitch = m_framebuffer_pitch; + resolution.width = m_framebuffer_width; + resolution.height = m_framebuffer_height; + copy_to_user(user_resolution, &resolution); return 0; } case FB_IOCTL_SET_RESOLUTION: { - auto* resolution = (FBResolution*)arg; - if (!Process::current()->validate_read_typed(resolution) || !Process::current()->validate_write_typed(resolution)) + auto* user_resolution = (FBResolution*)arg; + if (!Process::current()->validate_read_typed(user_resolution) || !Process::current()->validate_write_typed(user_resolution)) return -EFAULT; - resolution->pitch = m_framebuffer_pitch; - resolution->width = m_framebuffer_width; - resolution->height = m_framebuffer_height; + FBResolution resolution; + resolution.pitch = m_framebuffer_pitch; + resolution.width = m_framebuffer_width; + resolution.height = m_framebuffer_height; + copy_to_user(user_resolution, &resolution); return 0; } default: diff --git a/Kernel/Net/IPv4Socket.cpp b/Kernel/Net/IPv4Socket.cpp index c47913abaa..f38d3371f6 100644 --- a/Kernel/Net/IPv4Socket.cpp +++ b/Kernel/Net/IPv4Socket.cpp @@ -466,6 +466,8 @@ int IPv4Socket::ioctl(FileDescription&, unsigned request, FlatPtr arg) { REQUIRE_PROMISE(inet); + SmapDisabler disabler; + auto ioctl_route = [request, arg]() { auto* route = (rtentry*)arg; if (!Process::current()->validate_read_typed(route)) diff --git a/Kernel/Syscalls/ioctl.cpp b/Kernel/Syscalls/ioctl.cpp index 140997e177..52af998816 100644 --- a/Kernel/Syscalls/ioctl.cpp +++ b/Kernel/Syscalls/ioctl.cpp @@ -34,7 +34,6 @@ int Process::sys$ioctl(int fd, unsigned request, FlatPtr arg) auto description = file_description(fd); if (!description) return -EBADF; - SmapDisabler disabler; return description->file().ioctl(*description, request, arg); } diff --git a/Kernel/TTY/TTY.cpp b/Kernel/TTY/TTY.cpp index 2f1fde1675..de0e4477b0 100644 --- a/Kernel/TTY/TTY.cpp +++ b/Kernel/TTY/TTY.cpp @@ -284,8 +284,8 @@ int TTY::ioctl(FileDescription&, unsigned request, FlatPtr arg) REQUIRE_PROMISE(tty); auto& current_process = *Process::current(); pid_t pgid; - termios* tp; - winsize* ws; + termios* user_termios; + winsize* user_winsize; #if 0 // FIXME: When should we block things? @@ -313,22 +313,26 @@ int TTY::ioctl(FileDescription&, unsigned request, FlatPtr arg) } m_pgid = pgid; return 0; - case TCGETS: - tp = reinterpret_cast(arg); - if (!current_process.validate_write(tp, sizeof(termios))) + case TCGETS: { + user_termios = reinterpret_cast(arg); + if (!current_process.validate_write(user_termios, sizeof(termios))) return -EFAULT; - *tp = m_termios; + copy_to_user(user_termios, &m_termios); return 0; + } case TCSETS: case TCSETSF: - case TCSETSW: - tp = reinterpret_cast(arg); - if (!current_process.validate_read(tp, sizeof(termios))) + case TCSETSW: { + user_termios = reinterpret_cast(arg); + if (!current_process.validate_read(user_termios, sizeof(termios))) return -EFAULT; - set_termios(*tp); + termios termios; + copy_from_user(&termios, user_termios); + set_termios(termios); if (request == TCSETSF) flush_input(); return 0; + } case TCFLSH: // Serenity's TTY implementation does not use an output buffer, so ignore TCOFLUSH. if (arg == TCIFLUSH || arg == TCIOFLUSH) { @@ -338,22 +342,29 @@ int TTY::ioctl(FileDescription&, unsigned request, FlatPtr arg) } return 0; case TIOCGWINSZ: - ws = reinterpret_cast(arg); - if (!current_process.validate_write(ws, sizeof(winsize))) + user_winsize = reinterpret_cast(arg); + if (!current_process.validate_write(user_winsize, sizeof(winsize))) return -EFAULT; - ws->ws_row = m_rows; - ws->ws_col = m_columns; + winsize ws; + ws.ws_row = m_rows; + ws.ws_col = m_columns; + ws.ws_xpixel = 0; + ws.ws_ypixel = 0; + copy_to_user(user_winsize, &ws); return 0; - case TIOCSWINSZ: - ws = reinterpret_cast(arg); - if (!current_process.validate_read(ws, sizeof(winsize))) + case TIOCSWINSZ: { + user_winsize = reinterpret_cast(arg); + if (!current_process.validate_read(user_winsize, sizeof(winsize))) return -EFAULT; - if (ws->ws_col == m_columns && ws->ws_row == m_rows) + winsize ws; + copy_from_user(&ws, user_winsize); + if (ws.ws_col == m_columns && ws.ws_row == m_rows) return 0; - m_rows = ws->ws_row; - m_columns = ws->ws_col; + m_rows = ws.ws_row; + m_columns = ws.ws_col; generate_signal(SIGWINCH); return 0; + } case TIOCSCTTY: current_process.set_tty(this); return 0;