From 52511fd6cc1f09f591a5c387378b73fb3328d020 Mon Sep 17 00:00:00 2001 From: no92 Date: Sat, 17 Feb 2024 21:09:08 +0100 Subject: [PATCH] gfx: only expose the device over mbus after DRM setup completes To avoid use-after-free, the commit() function was changed to consume the AtomicState. This works well with the semantics of this, as capture takes a reference, but capture consumes it. --- core/drm/include/core/drm/core.hpp | 2 +- core/drm/src/ioctl.cpp | 10 +++++----- drivers/gfx/bochs/src/bochs.hpp | 6 +++--- drivers/gfx/bochs/src/main.cpp | 18 +++++++++--------- drivers/gfx/plainfb/src/main.cpp | 18 ++++++++++-------- drivers/gfx/plainfb/src/plainfb.hpp | 6 +++--- drivers/gfx/virtio/src/main.cpp | 18 ++++++++++-------- drivers/gfx/virtio/src/virtio.hpp | 6 +++--- drivers/gfx/vmware/src/main.cpp | 19 +++++++++++-------- drivers/gfx/vmware/src/vmware.hpp | 6 +++--- 10 files changed, 58 insertions(+), 51 deletions(-) diff --git a/core/drm/include/core/drm/core.hpp b/core/drm/include/core/drm/core.hpp index 64f76da3e..775f135f0 100644 --- a/core/drm/include/core/drm/core.hpp +++ b/core/drm/include/core/drm/core.hpp @@ -143,7 +143,7 @@ struct Configuration { virtual bool capture(std::vector assignment, std::unique_ptr &state) = 0; virtual void dispose() = 0; - virtual void commit(std::unique_ptr &state) = 0; + virtual void commit(std::unique_ptr state) = 0; auto waitForCompletion() { return _ev.wait(); diff --git a/core/drm/src/ioctl.cpp b/core/drm/src/ioctl.cpp index 1b39168f4..28dc58d31 100644 --- a/core/drm/src/ioctl.cpp +++ b/core/drm/src/ioctl.cpp @@ -497,7 +497,7 @@ drm_core::File::ioctl(void *object, uint32_t id, helix_ng::RecvInlineResult msg, auto state = self->_device->atomicState(); auto valid = config->capture(assignments, state); assert(valid); - config->commit(state); + config->commit(std::move(state)); co_await config->waitForCompletion(); @@ -530,7 +530,7 @@ drm_core::File::ioctl(void *object, uint32_t id, helix_ng::RecvInlineResult msg, auto state = self->_device->atomicState(); auto valid = config->capture(assignments, state); assert(valid); - config->commit(state); + config->commit(std::move(state)); co_await config->waitForCompletion(); self->_retirePageFlip(req->drm_cookie(), crtc->id()); @@ -621,7 +621,7 @@ drm_core::File::ioctl(void *object, uint32_t id, helix_ng::RecvInlineResult msg, auto state = self->_device->atomicState(); auto valid = config->capture(assignments, state); assert(valid); - config->commit(state); + config->commit(std::move(state)); co_await config->waitForCompletion(); @@ -819,7 +819,7 @@ drm_core::File::ioctl(void *object, uint32_t id, helix_ng::RecvInlineResult msg, auto valid = config->capture(assignments, state); assert(valid); - config->commit(state); + config->commit(std::move(state)); co_await config->waitForCompletion(); resp.set_error(managarm::fs::Errors::SUCCESS); @@ -1014,7 +1014,7 @@ drm_core::File::ioctl(void *object, uint32_t id, helix_ng::RecvInlineResult msg, if(!(req->drm_flags() & DRM_MODE_ATOMIC_TEST_ONLY)) { if(logDrmRequests) std::cout << "\tCommitting configuration ..." << std::endl; - config->commit(state); + config->commit(std::move(state)); co_await config->waitForCompletion(); } diff --git a/drivers/gfx/bochs/src/bochs.hpp b/drivers/gfx/bochs/src/bochs.hpp index e20441c8b..85211c39a 100644 --- a/drivers/gfx/bochs/src/bochs.hpp +++ b/drivers/gfx/bochs/src/bochs.hpp @@ -20,10 +20,10 @@ struct GfxDevice final : drm_core::Device, std::enable_shared_from_this assignment, std::unique_ptr &state) override; void dispose() override; - void commit(std::unique_ptr &state) override; + void commit(std::unique_ptr state) override; private: - async::detached _doCommit(std::unique_ptr &state); + async::detached _doCommit(std::unique_ptr state); GfxDevice *_device; }; @@ -90,7 +90,7 @@ struct GfxDevice final : drm_core::Device, std::enable_shared_from_this> initialize(); std::unique_ptr createConfiguration() override; std::pair, uint32_t> createDumb(uint32_t width, uint32_t height, uint32_t bpp) override; diff --git a/drivers/gfx/bochs/src/main.cpp b/drivers/gfx/bochs/src/main.cpp index ec1458c14..d05b4edf5 100644 --- a/drivers/gfx/bochs/src/main.cpp +++ b/drivers/gfx/bochs/src/main.cpp @@ -45,7 +45,7 @@ GfxDevice::GfxDevice(protocols::hw::Device hw_device, _operational = arch::global_io; } -async::detached GfxDevice::initialize() { +async::result> GfxDevice::initialize() { std::vector assignments; _operational.store(regs::index, (uint16_t)RegisterIndex::id); @@ -120,10 +120,9 @@ async::detached GfxDevice::initialize() { auto config = createConfiguration(); auto state = atomicState(); assert(config->capture(assignments, state)); - config->commit(state); - co_await config->waitForCompletion(); + config->commit(std::move(state)); - co_return; + co_return std::move(config); } std::unique_ptr GfxDevice::createConfiguration() { @@ -240,15 +239,15 @@ void GfxDevice::Configuration::dispose() { } -void GfxDevice::Configuration::commit(std::unique_ptr &state) { - _doCommit(state); - +void GfxDevice::Configuration::commit(std::unique_ptr state) { _device->_theCrtc->setDrmState(state->crtc(_device->_theCrtc->id())); _device->_theConnector->setDrmState(state->connector(_device->_theConnector->id())); _device->_primaryPlane->setDrmState(state->plane(_device->_primaryPlane->id())); + + _doCommit(std::move(state)); } -async::detached GfxDevice::Configuration::_doCommit(std::unique_ptr &state) { +async::detached GfxDevice::Configuration::_doCommit(std::unique_ptr state) { if(logCommits) std::cout << "gfx-bochs: Committing configuration" << std::endl; @@ -433,7 +432,7 @@ async::detached bindController(mbus::Entity entity) { auto gfx_device = std::make_shared(std::move(pci_device), std::move(bar), actual_pointer); - gfx_device->initialize(); + auto config = co_await gfx_device->initialize(); // Create an mbus object for the device. auto root = co_await mbus::Instance::global().getRoot(); @@ -453,6 +452,7 @@ async::detached bindController(mbus::Entity entity) { co_return std::move(remote_lane); }); + co_await config->waitForCompletion(); co_await root.createObject("gfx_bochs", descriptor, std::move(handler)); } diff --git a/drivers/gfx/plainfb/src/main.cpp b/drivers/gfx/plainfb/src/main.cpp index 0b440d49f..7bb0253f7 100644 --- a/drivers/gfx/plainfb/src/main.cpp +++ b/drivers/gfx/plainfb/src/main.cpp @@ -38,7 +38,7 @@ GfxDevice::GfxDevice(protocols::hw::Device hw_device, } } -async::detached GfxDevice::initialize() { +async::result> GfxDevice::initialize() { // Setup planes, encoders and CRTCs (i.e. the static entities). _plane = std::make_shared(this, Plane::PlaneType::PRIMARY); _theCrtc = std::make_shared(this); @@ -117,8 +117,9 @@ async::detached GfxDevice::initialize() { auto config = createConfiguration(); auto state = atomicState(); assert(config->capture(assignments, state)); - config->commit(state); - co_await config->waitForCompletion(); + config->commit(std::move(state)); + + co_return config; } std::unique_ptr GfxDevice::createConfiguration() { @@ -197,15 +198,15 @@ void GfxDevice::Configuration::dispose() { } -void GfxDevice::Configuration::commit(std::unique_ptr &state) { - _dispatch(state); - +void GfxDevice::Configuration::commit(std::unique_ptr state) { _device->_theCrtc->setDrmState(state->crtc(_device->_theCrtc->id())); _device->_theConnector->setDrmState(state->connector(_device->_theConnector->id())); _device->_plane->setDrmState(state->plane(_device->_plane->id())); + + _dispatch(std::move(state)); } -async::detached GfxDevice::Configuration::_dispatch(std::unique_ptr &state) { +async::detached GfxDevice::Configuration::_dispatch(std::unique_ptr state) { auto crtc_state = state->crtc(_device->_theCrtc->id()); if(crtc_state->mode != nullptr) { @@ -366,7 +367,7 @@ async::detached bindController(mbus::Entity entity) { auto gfx_device = std::make_shared(std::move(hw_device), info.width, info.height, info.pitch, helix::Mapping{fb_memory, 0, info.pitch * info.height}); - gfx_device->initialize(); + auto config = co_await gfx_device->initialize(); // Create an mbus object for the device. auto root = co_await mbus::Instance::global().getRoot(); @@ -386,6 +387,7 @@ async::detached bindController(mbus::Entity entity) { co_return std::move(remote_lane); }); + co_await config->waitForCompletion(); co_await root.createObject("gfx_plainfb", descriptor, std::move(handler)); } diff --git a/drivers/gfx/plainfb/src/plainfb.hpp b/drivers/gfx/plainfb/src/plainfb.hpp index 978045763..e6a5353ec 100644 --- a/drivers/gfx/plainfb/src/plainfb.hpp +++ b/drivers/gfx/plainfb/src/plainfb.hpp @@ -23,10 +23,10 @@ struct GfxDevice final : drm_core::Device, std::enable_shared_from_this assignment, std::unique_ptr &state) override; void dispose() override; - void commit(std::unique_ptr &state) override; + void commit(std::unique_ptr state) override; private: - async::detached _dispatch(std::unique_ptr &state); + async::detached _dispatch(std::unique_ptr state); GfxDevice *_device; }; @@ -91,7 +91,7 @@ struct GfxDevice final : drm_core::Device, std::enable_shared_from_this> initialize(); std::unique_ptr createConfiguration() override; std::pair, uint32_t> createDumb(uint32_t width, uint32_t height, uint32_t bpp) override; diff --git a/drivers/gfx/virtio/src/main.cpp b/drivers/gfx/virtio/src/main.cpp index 8a3c89cfa..4b8be71c0 100644 --- a/drivers/gfx/virtio/src/main.cpp +++ b/drivers/gfx/virtio/src/main.cpp @@ -65,7 +65,7 @@ struct AwaitableRequest : virtio_core::Request { GfxDevice::GfxDevice(std::unique_ptr transport) : _transport{std::move(transport)}, _claimedDevice{false} { } -async::detached GfxDevice::initialize() { +async::result> GfxDevice::initialize() { if(_transport->checkDeviceFeature(VIRTIO_GPU_F_VIRGL)) { _transport->acknowledgeDriverFeature(VIRTIO_GPU_F_VIRGL); _virgl3D = true; @@ -173,8 +173,9 @@ async::detached GfxDevice::initialize() { auto config = createConfiguration(); auto state = atomicState(); assert(config->capture(assignments, state)); - config->commit(state); - co_await config->waitForCompletion(); + config->commit(std::move(state)); + + co_return config; } std::unique_ptr GfxDevice::createConfiguration() { @@ -257,9 +258,7 @@ void GfxDevice::Configuration::dispose() { } -void GfxDevice::Configuration::commit(std::unique_ptr &state) { - _dispatch(state); - +void GfxDevice::Configuration::commit(std::unique_ptr state) { for(auto &[_, cs] : state->crtc_states()) { cs->crtc().lock()->setDrmState(cs); } @@ -271,9 +270,11 @@ void GfxDevice::Configuration::commit(std::unique_ptr &st for(auto &[_, cs] : state->connector_states()) { cs->connector->setDrmState(cs); } + + _dispatch(std::move(state)); } -async::detached GfxDevice::Configuration::_dispatch(std::unique_ptr &state) { +async::detached GfxDevice::Configuration::_dispatch(std::unique_ptr state) { if(!_device->_claimedDevice) { co_await _device->_transport->hwDevice().claimDevice(); _device->_claimedDevice = true; @@ -455,7 +456,7 @@ async::result doBind(mbus::Entity base_entity) { virtio_core::DiscoverMode::modernOnly); auto gfx_device = std::make_shared(std::move(transport)); - gfx_device->initialize(); + auto config = co_await gfx_device->initialize(); // Create an mbus object for the device. auto root = co_await mbus::Instance::global().getRoot(); @@ -475,6 +476,7 @@ async::result doBind(mbus::Entity base_entity) { co_return std::move(remote_lane); }); + co_await config->waitForCompletion(); co_await root.createObject("gfx_virtio", descriptor, std::move(handler)); baseDeviceMap.insert({base_entity.getId(), gfx_device}); } diff --git a/drivers/gfx/virtio/src/virtio.hpp b/drivers/gfx/virtio/src/virtio.hpp index 4e1d76e24..2723fe9fb 100644 --- a/drivers/gfx/virtio/src/virtio.hpp +++ b/drivers/gfx/virtio/src/virtio.hpp @@ -38,10 +38,10 @@ struct GfxDevice final : drm_core::Device, std::enable_shared_from_this assignment, std::unique_ptr &state) override; void dispose() override; - void commit(std::unique_ptr &state) override; + void commit(std::unique_ptr state) override; private: - async::detached _dispatch(std::unique_ptr &state); + async::detached _dispatch(std::unique_ptr state); GfxDevice *_device; }; @@ -116,7 +116,7 @@ struct GfxDevice final : drm_core::Device, std::enable_shared_from_this transport); - async::detached initialize(); + async::result> initialize(); std::unique_ptr createConfiguration() override; std::pair, uint32_t> createDumb(uint32_t width, uint32_t height, uint32_t bpp) override; diff --git a/drivers/gfx/vmware/src/main.cpp b/drivers/gfx/vmware/src/main.cpp index 288cb63c9..4570047c9 100644 --- a/drivers/gfx/vmware/src/main.cpp +++ b/drivers/gfx/vmware/src/main.cpp @@ -52,7 +52,7 @@ GfxDevice::GfxDevice(protocols::hw::Device hw_dev, _operational = arch::io_space{io_base}; } -async::detached GfxDevice::initialize() { +async::result> GfxDevice::initialize() { auto pci_info = co_await _hwDev.getPciInfo(); // negotiate version @@ -172,8 +172,9 @@ async::detached GfxDevice::initialize() { auto state = atomicState(); auto valid = config->capture(assignments, state); assert(valid); - config->commit(state); - co_await config->waitForCompletion(); + config->commit(std::move(state)); + + co_return std::move(config); } std::shared_ptr GfxDevice::createFrameBuffer(std::shared_ptr base_bo, @@ -556,15 +557,15 @@ void GfxDevice::Configuration::dispose() { } -void GfxDevice::Configuration::commit(std::unique_ptr & state) { - commitConfiguration(state); - +void GfxDevice::Configuration::commit(std::unique_ptr state) { _device->_crtc->setDrmState(state->crtc(_device->_crtc->id())); _device->_primaryPlane->setDrmState(state->plane(_device->_primaryPlane->id())); _device->_cursorPlane->setDrmState(state->plane(_device->_cursorPlane->id())); + + commitConfiguration(std::move(state)); } -async::detached GfxDevice::Configuration::commitConfiguration(std::unique_ptr & state) { +async::detached GfxDevice::Configuration::commitConfiguration(std::unique_ptr state) { auto primary_plane_state = state->plane(_device->_primaryPlane->id()); auto cursor_plane_state = state->plane(_device->_cursorPlane->id()); auto crtc_state = state->crtc(_device->_crtc->id()); @@ -741,7 +742,7 @@ async::result setupDevice(mbus::Entity entity) { helix::Mapping{fifo_bar, 0, fifo_bar_info.length}, std::move(io_bar), io_bar_info.address); - gfx_device->initialize(); + auto config = co_await gfx_device->initialize(); auto root = co_await mbus::Instance::global().getRoot(); @@ -759,6 +760,8 @@ async::result setupDevice(mbus::Entity entity) { co_return std::move(remote_lane); }); + + co_await config->waitForCompletion(); co_await root.createObject("gfx_vmware", descriptor, std::move(handler)); } diff --git a/drivers/gfx/vmware/src/vmware.hpp b/drivers/gfx/vmware/src/vmware.hpp index 8570192e7..90f9b1eb0 100644 --- a/drivers/gfx/vmware/src/vmware.hpp +++ b/drivers/gfx/vmware/src/vmware.hpp @@ -22,10 +22,10 @@ struct GfxDevice final : drm_core::Device, std::enable_shared_from_this assignment, std::unique_ptr & state) override; void dispose() override; - void commit(std::unique_ptr & state) override; + void commit(std::unique_ptr state) override; private: - async::detached commitConfiguration(std::unique_ptr & state); + async::detached commitConfiguration(std::unique_ptr state); GfxDevice *_device; @@ -116,7 +116,7 @@ struct GfxDevice final : drm_core::Device, std::enable_shared_from_this> initialize(); std::unique_ptr createConfiguration() override; std::pair, uint32_t> createDumb(uint32_t width, uint32_t height, uint32_t bpp) override;