From 977aa8131055ef4185c6f4fbde6b1412f063f542 Mon Sep 17 00:00:00 2001 From: Liav A Date: Fri, 10 Jun 2022 14:16:28 +0300 Subject: [PATCH] Kernel+Userland: Add ioctl to set process ownership of DisplayConnector Now that the infrastructure of the Graphics subsystem is quite stable, it is time to try to fix a long-standing problem, which is the lack of locking on display connector devices. Reading and writing from multiple processes to a framebuffer controlled by the display connector is not a huge problem - it could be solved with POSIX locking. The real problem is some program that will try to do ioctl operations on a display connector without the WindowServer being aware of that which can lead to very bad situations, for example - assuming a framebuffer is encoded at a known resolution and certain display timings, but another process changed the ModeSetting of the display connector, leading to inconsistency on the properties of the current ModeSetting. To solve this, there's a new "master" ioctl to take "ownership" and another one to release that ownership of a display connector device. To ensure we will not hold a Process object forever just because it has an ownership over a display connector, we hold it with a weak reference, and if the process is gone, someone else can take an ownership. --- Kernel/API/Graphics.h | 10 +++ Kernel/Graphics/DisplayConnector.cpp | 70 +++++++++++++++++++++ Kernel/Graphics/DisplayConnector.h | 5 ++ Userland/Libraries/LibC/sys/ioctl_numbers.h | 4 ++ Userland/Services/WindowServer/main.cpp | 20 +++--- 5 files changed, 101 insertions(+), 8 deletions(-) diff --git a/Kernel/API/Graphics.h b/Kernel/API/Graphics.h index 36948e3e6168d8..50cd983a481119 100644 --- a/Kernel/API/Graphics.h +++ b/Kernel/API/Graphics.h @@ -50,6 +50,16 @@ ALWAYS_INLINE int graphics_connector_get_head_edid(int fd, GraphicsHeadEDID* inf return 0; } +ALWAYS_INLINE int graphics_connector_set_responsible(int fd) +{ + return ioctl(fd, GRAPHICS_IOCTL_SET_RESPONSIBLE, nullptr); +} + +ALWAYS_INLINE int graphics_connector_unset_responsible(int fd) +{ + return ioctl(fd, GRAPHICS_IOCTL_UNSET_RESPONSIBLE, nullptr); +} + ALWAYS_INLINE int fb_get_head_vertical_offset_buffer(int fd, GraphicsHeadVerticalOffset* vertical_offset) { return ioctl(fd, GRAPHICS_IOCTL_GET_HEAD_VERTICAL_OFFSET_BUFFER, vertical_offset); diff --git a/Kernel/Graphics/DisplayConnector.cpp b/Kernel/Graphics/DisplayConnector.cpp index 7d42d8db05f4ed..f9d82ec9599e11 100644 --- a/Kernel/Graphics/DisplayConnector.cpp +++ b/Kernel/Graphics/DisplayConnector.cpp @@ -231,11 +231,81 @@ ErrorOr DisplayConnector::get_edid() const return ByteBuffer::copy(m_edid_bytes, sizeof(m_edid_bytes)); } +struct GraphicsIOCtlChecker { + unsigned ioctl_number; + StringView name; + bool requires_ownership { false }; +}; + +static constexpr GraphicsIOCtlChecker s_checkers[] = { + { GRAPHICS_IOCTL_GET_PROPERTIES, "GRAPHICS_IOCTL_GET_PROPERTIES"sv, false }, + { GRAPHICS_IOCTL_SET_HEAD_VERTICAL_OFFSET_BUFFER, "GRAPHICS_IOCTL_SET_HEAD_VERTICAL_OFFSET_BUFFER"sv, true }, + { GRAPHICS_IOCTL_GET_HEAD_VERTICAL_OFFSET_BUFFER, "GRAPHICS_IOCTL_GET_HEAD_VERTICAL_OFFSET_BUFFER"sv, false }, + { GRAPHICS_IOCTL_FLUSH_HEAD_BUFFERS, "GRAPHICS_IOCTL_FLUSH_HEAD_BUFFERS"sv, true }, + { GRAPHICS_IOCTL_FLUSH_HEAD, "GRAPHICS_IOCTL_FLUSH_HEAD"sv, true }, + { GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING, "GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING"sv, true }, + { GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING, "GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING"sv, false }, + { GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING, "GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING"sv, true }, + { GRAPHICS_IOCTL_SET_RESPONSIBLE, "GRAPHICS_IOCTL_SET_RESPONSIBLE"sv, false }, + { GRAPHICS_IOCTL_UNSET_RESPONSIBLE, "GRAPHICS_IOCTL_UNSET_RESPONSIBLE"sv, true }, +}; + +static StringView ioctl_to_stringview(unsigned request) +{ + for (auto& checker : s_checkers) { + if (checker.ioctl_number == request) + return checker.name; + } + VERIFY_NOT_REACHED(); +} + +bool DisplayConnector::ioctl_requires_ownership(unsigned request) const +{ + for (auto& checker : s_checkers) { + if (checker.ioctl_number == request) + return checker.requires_ownership; + } + VERIFY_NOT_REACHED(); +} + ErrorOr DisplayConnector::ioctl(OpenFileDescription&, unsigned request, Userspace arg) { TRY(Process::current().require_promise(Pledge::video)); + // Note: We only allow to set responsibility on a DisplayConnector, + // get the current ModeSetting or the hardware framebuffer properties without the + // need of having an established responsibility on a DisplayConnector. + if (ioctl_requires_ownership(request)) { + auto process = m_responsible_process.strong_ref(); + if (!process || process.ptr() != &Process::current()) { + dbgln("DisplayConnector::ioctl: {} requires ownership over the device", ioctl_to_stringview(request)); + return Error::from_errno(EPERM); + } + } + switch (request) { + case GRAPHICS_IOCTL_SET_RESPONSIBLE: { + SpinlockLocker locker(m_responsible_process_lock); + auto process = m_responsible_process.strong_ref(); + // Note: If there's already a process being responsible, just return an error. + // We could technically return 0 if the the requesting process is already + // was set to be responsible for this DisplayConnector, but it servicing no + // no good purpose and should be considered a bug if this happens anyway. + if (process) + return Error::from_errno(EPERM); + m_responsible_process = Process::current(); + return {}; + } + case GRAPHICS_IOCTL_UNSET_RESPONSIBLE: { + SpinlockLocker locker(m_responsible_process_lock); + auto process = m_responsible_process.strong_ref(); + if (!process) + return Error::from_errno(ESRCH); + if (process.ptr() != &Process::current()) + return Error::from_errno(EPERM); + m_responsible_process.clear(); + return {}; + } case GRAPHICS_IOCTL_GET_PROPERTIES: { auto user_properties = static_ptr_cast(arg); GraphicsConnectorProperties properties {}; diff --git a/Kernel/Graphics/DisplayConnector.h b/Kernel/Graphics/DisplayConnector.h index e45dd7defb3e61..d486b40f0cdd93 100644 --- a/Kernel/Graphics/DisplayConnector.h +++ b/Kernel/Graphics/DisplayConnector.h @@ -148,6 +148,8 @@ class DisplayConnector : public CharacterDevice { virtual void will_be_destroyed() override; virtual void after_inserting() override; + bool ioctl_requires_ownership(unsigned request) const; + OwnPtr m_framebuffer_region; OwnPtr m_fake_writes_framebuffer_region; u8* m_framebuffer_data {}; @@ -162,6 +164,9 @@ class DisplayConnector : public CharacterDevice { private: RefPtr m_shared_framebuffer_vmobject; + WeakPtr m_responsible_process; + Spinlock m_responsible_process_lock; + IntrusiveListNode> m_list_node; }; } diff --git a/Userland/Libraries/LibC/sys/ioctl_numbers.h b/Userland/Libraries/LibC/sys/ioctl_numbers.h index 156ffad5300949..1b9b433c73c05e 100644 --- a/Userland/Libraries/LibC/sys/ioctl_numbers.h +++ b/Userland/Libraries/LibC/sys/ioctl_numbers.h @@ -101,6 +101,8 @@ enum IOCtlNumber { GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING, GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING, GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING, + GRAPHICS_IOCTL_SET_RESPONSIBLE, + GRAPHICS_IOCTL_UNSET_RESPONSIBLE, KEYBOARD_IOCTL_GET_NUM_LOCK, KEYBOARD_IOCTL_SET_NUM_LOCK, KEYBOARD_IOCTL_GET_CAPS_LOCK, @@ -160,6 +162,8 @@ enum IOCtlNumber { #define GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING GRAPHICS_IOCTL_SET_HEAD_MODE_SETTING #define GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING GRAPHICS_IOCTL_GET_HEAD_MODE_SETTING #define GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING GRAPHICS_IOCTL_SET_SAFE_HEAD_MODE_SETTING +#define GRAPHICS_IOCTL_SET_RESPONSIBLE GRAPHICS_IOCTL_SET_RESPONSIBLE +#define GRAPHICS_IOCTL_UNSET_RESPONSIBLE GRAPHICS_IOCTL_UNSET_RESPONSIBLE #define KEYBOARD_IOCTL_GET_NUM_LOCK KEYBOARD_IOCTL_GET_NUM_LOCK #define KEYBOARD_IOCTL_SET_NUM_LOCK KEYBOARD_IOCTL_SET_NUM_LOCK #define KEYBOARD_IOCTL_GET_CAPS_LOCK KEYBOARD_IOCTL_GET_CAPS_LOCK diff --git a/Userland/Services/WindowServer/main.cpp b/Userland/Services/WindowServer/main.cpp index a5d6339a9079a8..8dfa5640e12698 100644 --- a/Userland/Services/WindowServer/main.cpp +++ b/Userland/Services/WindowServer/main.cpp @@ -9,6 +9,7 @@ #include "EventLoop.h" #include "Screen.h" #include "WindowManager.h" +#include #include #include #include @@ -68,7 +69,7 @@ ErrorOr serenity_main(Main::Arguments) WindowServer::ScreenLayout screen_layout; String error_msg; - auto add_unconfigured_display_connector_devices = [&]() { + auto add_unconfigured_display_connector_devices = [&]() -> ErrorOr { // Enumerate the /dev/gpu/connectorX devices and try to set up any ones we find that we haven't already used Core::DirIterator di("/dev/gpu", Core::DirIterator::SkipParentAndBaseDir); while (di.has_next()) { @@ -78,18 +79,23 @@ ErrorOr serenity_main(Main::Arguments) auto full_path = String::formatted("/dev/gpu/{}", path); if (!Core::File::is_device(full_path)) continue; + auto display_connector_fd = TRY(Core::System::open(full_path, O_RDWR | O_CLOEXEC)); + if (int rc = graphics_connector_set_responsible(display_connector_fd); rc != 0) + return Error::from_syscall("graphics_connector_set_responsible"sv, rc); + TRY(Core::System::close(display_connector_fd)); if (fb_devices_configured.find(full_path) != fb_devices_configured.end()) continue; if (!screen_layout.try_auto_add_display_connector(full_path)) dbgln("Could not auto-add display connector device {} to screen layout", full_path); } + return {}; }; - auto apply_and_generate_generic_screen_layout = [&]() { + auto apply_and_generate_generic_screen_layout = [&]() -> ErrorOr { screen_layout = {}; fb_devices_configured = {}; - add_unconfigured_display_connector_devices(); + TRY(add_unconfigured_display_connector_devices()); if (!WindowServer::Screen::apply_layout(move(screen_layout), error_msg)) { dbgln("Failed to apply generated fallback screen layout: {}", error_msg); return false; @@ -104,17 +110,15 @@ ErrorOr serenity_main(Main::Arguments) if (screen_info.mode == WindowServer::ScreenLayout::Screen::Mode::Device) fb_devices_configured.set(screen_info.device.value()); - add_unconfigured_display_connector_devices(); + TRY(add_unconfigured_display_connector_devices()); if (!WindowServer::Screen::apply_layout(move(screen_layout), error_msg)) { dbgln("Error applying screen layout: {}", error_msg); - if (!apply_and_generate_generic_screen_layout()) - return 1; + TRY(apply_and_generate_generic_screen_layout()); } } else { dbgln("Error loading screen configuration: {}", error_msg); - if (!apply_and_generate_generic_screen_layout()) - return 1; + TRY(apply_and_generate_generic_screen_layout()); } }