From 0d648eae62636d6bb9e0145cdda6183ee4f36315 Mon Sep 17 00:00:00 2001 From: Stefan Kalkowski Date: Fri, 10 Jan 2025 21:17:19 +0100 Subject: [PATCH] hw: sanitize kernel's signal datastructures * Move all Kernel::Signal_* structures to kernel/signal.* * Remove return value of kill_signal_context, which wasn't evaluated * Remove Kernel::Signal_context::can_kill * Remove Kernel::Signal_context::can_submit * Remove Kernel::Signal_receiver::can_add_handler * Turn nullptr into cxx nullptr instead of just zero * Turn boolean values into true/false instead of one/zero * Always add to signal FIFO also if submit counter cannot get increased enough Fix genodelabs/genode#5416 --- repos/base-hw/include/kernel/interface.h | 7 +-- repos/base-hw/lib/mk/core-hw.inc | 2 +- repos/base-hw/src/core/kernel/irq.h | 6 +- .../kernel/{signal_receiver.cc => signal.cc} | 61 ++++++------------- .../kernel/{signal_receiver.h => signal.h} | 20 ++---- repos/base-hw/src/core/kernel/thread.cc | 52 +++------------- repos/base-hw/src/core/kernel/thread.h | 2 +- repos/base-hw/src/core/kernel/vm.h | 2 +- repos/base-hw/src/core/pager.h | 2 +- .../src/core/signal_source_component.h | 2 +- 10 files changed, 41 insertions(+), 115 deletions(-) rename repos/base-hw/src/core/kernel/{signal_receiver.cc => signal.cc} (83%) rename repos/base-hw/src/core/kernel/{signal_receiver.h => signal.h} (93%) diff --git a/repos/base-hw/include/kernel/interface.h b/repos/base-hw/include/kernel/interface.h index 0925f0c6dc..0bef5bffa8 100644 --- a/repos/base-hw/include/kernel/interface.h +++ b/repos/base-hw/include/kernel/interface.h @@ -382,13 +382,10 @@ namespace Kernel { * Halt processing of a signal context synchronously * * \param context capability ID of the targeted signal context - * - * \retval 0 suceeded - * \retval -1 failed */ - inline int kill_signal_context(capid_t const context) + inline void kill_signal_context(capid_t const context) { - return (int)call(call_id_kill_signal_context(), context); + call(call_id_kill_signal_context(), context); } /** diff --git a/repos/base-hw/lib/mk/core-hw.inc b/repos/base-hw/lib/mk/core-hw.inc index 26f3711358..832c4de40d 100644 --- a/repos/base-hw/lib/mk/core-hw.inc +++ b/repos/base-hw/lib/mk/core-hw.inc @@ -54,7 +54,7 @@ SRC_CC += kernel/ipc_node.cc SRC_CC += kernel/irq.cc SRC_CC += kernel/main.cc SRC_CC += kernel/object.cc -SRC_CC += kernel/signal_receiver.cc +SRC_CC += kernel/signal.cc SRC_CC += kernel/thread.cc SRC_CC += kernel/timer.cc SRC_CC += capability.cc diff --git a/repos/base-hw/src/core/kernel/irq.h b/repos/base-hw/src/core/kernel/irq.h index 9b8be0bf65..bdfb858fc1 100644 --- a/repos/base-hw/src/core/kernel/irq.h +++ b/repos/base-hw/src/core/kernel/irq.h @@ -20,7 +20,7 @@ #include /* core includes */ -#include +#include namespace Board { @@ -161,9 +161,7 @@ class Kernel::User_irq : public Kernel::Irq */ void occurred() override { - if (_context.can_submit(1)) { - _context.submit(1); - } + _context.submit(1); disable(); } diff --git a/repos/base-hw/src/core/kernel/signal_receiver.cc b/repos/base-hw/src/core/kernel/signal.cc similarity index 83% rename from repos/base-hw/src/core/kernel/signal_receiver.cc rename to repos/base-hw/src/core/kernel/signal.cc index 5c99894103..ad5017386a 100644 --- a/repos/base-hw/src/core/kernel/signal_receiver.cc +++ b/repos/base-hw/src/core/kernel/signal.cc @@ -1,18 +1,19 @@ /* * \brief Kernel backend for asynchronous inter-process communication * \author Martin Stein + * \author Stefan Kalkowski * \date 2012-11-30 */ /* - * Copyright (C) 2012-2019 Genode Labs GmbH + * Copyright (C) 2012-2025 Genode Labs GmbH * * This file is part of the Genode OS framework, which is distributed * under the terms of the GNU Affero General Public License version 3. */ /* core includes */ -#include +#include #include using namespace Kernel; @@ -26,7 +27,7 @@ void Signal_handler::cancel_waiting() { if (_receiver) { _receiver->_handler_cancelled(*this); - _receiver = 0; + _receiver = nullptr; } } @@ -71,28 +72,20 @@ void Signal_context::_deliverable() void Signal_context::_delivered() { _submits = 0; - _ack = 0; + _ack = false; } -void Signal_context::_killer_cancelled() { _killer = 0; } - - -bool Signal_context::can_submit(unsigned const n) const -{ - if (_killed || _submits >= (unsigned)~0 - n) - return false; - - return true; -} +void Signal_context::_killer_cancelled() { _killer = nullptr; } void Signal_context::submit(unsigned const n) { - if (_killed || _submits >= (unsigned)~0 - n) + if (_killed) return; - _submits += n; + if (_submits < ((unsigned)~0 - n)) + _submits += n; if (_ack) _deliverable(); @@ -105,32 +98,19 @@ void Signal_context::ack() return; if (!_killed) { - _ack = 1; + _ack = true; _deliverable(); return; } if (_killer) { - _killer->_context = 0; + _killer->_context = nullptr; _killer->_thread.signal_context_kill_done(); - _killer = 0; + _killer = nullptr; } } -bool Signal_context::can_kill() const -{ - /* check if in a kill operation or already killed */ - if (_killed) { - if (_ack) - return true; - - return false; - } - return true; -} - - void Signal_context::kill(Signal_context_killer &k) { /* check if in a kill operation or already killed */ @@ -139,13 +119,13 @@ void Signal_context::kill(Signal_context_killer &k) /* kill directly if there is no unacknowledged delivery */ if (_ack) { - _killed = 1; + _killed = true; return; } /* wait for delivery acknowledgement */ _killer = &k; - _killed = 1; + _killed = true; _killer->_context = this; _killer->_thread.signal_context_kill_pending(); } @@ -231,24 +211,17 @@ void Signal_receiver::_add_context(Signal_context &c) { _contexts.enqueue(c._contexts_fe); } -bool Signal_receiver::can_add_handler(Signal_handler const &h) const + +bool Signal_receiver::add_handler(Signal_handler &h) { if (h._receiver) return false; - return true; -} - - -void Signal_receiver::add_handler(Signal_handler &h) -{ - if (h._receiver) - return; - _handlers.enqueue(h._handlers_fe); h._receiver = this; h._thread.signal_wait_for_signal(); _listen(); + return true; } diff --git a/repos/base-hw/src/core/kernel/signal_receiver.h b/repos/base-hw/src/core/kernel/signal.h similarity index 93% rename from repos/base-hw/src/core/kernel/signal_receiver.h rename to repos/base-hw/src/core/kernel/signal.h index 6f051213bc..3fa729e481 100644 --- a/repos/base-hw/src/core/kernel/signal_receiver.h +++ b/repos/base-hw/src/core/kernel/signal.h @@ -1,18 +1,19 @@ /* * \brief Kernel backend for asynchronous inter-process communication * \author Martin Stein + * \author Stefan Kalkowski * \date 2012-11-30 */ /* - * Copyright (C) 2012-2017 Genode Labs GmbH + * Copyright (C) 2012-2025 Genode Labs GmbH * * This file is part of the Genode OS framework, which is distributed * under the terms of the GNU Affero General Public License version 3. */ -#ifndef _CORE__KERNEL__SIGNAL_RECEIVER_H_ -#define _CORE__KERNEL__SIGNAL_RECEIVER_H_ +#ifndef _CORE__KERNEL__SIGNAL_H_ +#define _CORE__KERNEL__SIGNAL_H_ /* Genode includes */ #include @@ -165,11 +166,7 @@ class Kernel::Signal_context * Submit the signal * * \param n number of submits - * - * \retval 0 succeeded - * \retval -1 failed */ - bool can_submit(unsigned const n) const; void submit(unsigned const n); /** @@ -180,12 +177,8 @@ class Kernel::Signal_context /** * Destruct context or prepare to do it as soon as delivery is done * - * \param killer object that shall receive progress reports - * - * \retval 0 succeeded - * \retval -1 failed + * \param k object that shall receive progress reports */ - bool can_kill() const; void kill(Signal_context_killer &k); /** @@ -270,8 +263,7 @@ class Kernel::Signal_receiver * \retval 0 succeeded * \retval -1 failed */ - bool can_add_handler(Signal_handler const &h) const; - void add_handler(Signal_handler &h); + bool add_handler(Signal_handler &h); /** * Syscall to create a signal receiver diff --git a/repos/base-hw/src/core/kernel/thread.cc b/repos/base-hw/src/core/kernel/thread.cc index 688afe1eb7..f749276bed 100644 --- a/repos/base-hw/src/core/kernel/thread.cc +++ b/repos/base-hw/src/core/kernel/thread.cc @@ -522,11 +522,8 @@ void Thread::timeout_triggered() { Signal_context * const c = pd().cap_tree().find(_timeout_sigid); - if (!c || !c->can_submit(1)) { - Genode::raw(*this, ": failed to submit timeout signal"); - return; - } - c->submit(1); + if (c) c->submit(1); + else Genode::warning(*this, ": failed to submit timeout signal"); } @@ -602,12 +599,11 @@ void Thread::_call_await_signal() return; } /* register handler at the receiver */ - if (!r->can_add_handler(_signal_handler)) { + if (!r->add_handler(_signal_handler)) { Genode::raw("failed to register handler at signal receiver"); user_arg_0(-1); return; } - r->add_handler(_signal_handler); user_arg_0(0); } @@ -624,11 +620,10 @@ void Thread::_call_pending_signal() } /* register handler at the receiver */ - if (!r->can_add_handler(_signal_handler)) { + if (!r->add_handler(_signal_handler)) { user_arg_0(-1); return; } - r->add_handler(_signal_handler); if (_state == AWAITS_SIGNAL) { _cancel_blocking(); @@ -663,20 +658,7 @@ void Thread::_call_submit_signal() { /* lookup signal context */ Signal_context * const c = pd().cap_tree().find((Kernel::capid_t)user_arg_1()); - if(!c) { - /* cannot submit unknown signal context */ - user_arg_0(-1); - return; - } - - /* trigger signal context */ - if (!c->can_submit((unsigned)user_arg_2())) { - Genode::raw("failed to submit signal context"); - user_arg_0(-1); - return; - } - c->submit((unsigned)user_arg_2()); - user_arg_0(0); + if(c) c->submit((unsigned)user_arg_2()); } @@ -684,13 +666,8 @@ void Thread::_call_ack_signal() { /* lookup signal context */ Signal_context * const c = pd().cap_tree().find((Kernel::capid_t)user_arg_1()); - if (!c) { - Genode::raw(*this, ": cannot ack unknown signal context"); - return; - } - - /* acknowledge */ - c->ack(); + if (c) c->ack(); + else Genode::warning(*this, ": cannot ack unknown signal context"); } @@ -698,19 +675,8 @@ void Thread::_call_kill_signal_context() { /* lookup signal context */ Signal_context * const c = pd().cap_tree().find((Kernel::capid_t)user_arg_1()); - if (!c) { - Genode::raw(*this, ": cannot kill unknown signal context"); - user_arg_0(-1); - return; - } - - /* kill signal context */ - if (!c->can_kill()) { - Genode::raw("failed to kill signal context"); - user_arg_0(-1); - return; - } - c->kill(_signal_context_killer); + if (c) c->kill(_signal_context_killer); + else Genode::warning(*this, ": cannot kill unknown signal context"); } diff --git a/repos/base-hw/src/core/kernel/thread.h b/repos/base-hw/src/core/kernel/thread.h index b7b5abfcba..74fc257250 100644 --- a/repos/base-hw/src/core/kernel/thread.h +++ b/repos/base-hw/src/core/kernel/thread.h @@ -20,7 +20,7 @@ /* base-hw core includes */ #include #include -#include +#include #include #include #include diff --git a/repos/base-hw/src/core/kernel/vm.h b/repos/base-hw/src/core/kernel/vm.h index 75be17063d..7b82c81803 100644 --- a/repos/base-hw/src/core/kernel/vm.h +++ b/repos/base-hw/src/core/kernel/vm.h @@ -18,7 +18,7 @@ /* core includes */ #include #include -#include +#include #include diff --git a/repos/base-hw/src/core/pager.h b/repos/base-hw/src/core/pager.h index 93f2e1ad86..b110ff4fea 100644 --- a/repos/base-hw/src/core/pager.h +++ b/repos/base-hw/src/core/pager.h @@ -22,7 +22,7 @@ #include /* core includes */ -#include +#include #include #include #include diff --git a/repos/base-hw/src/core/signal_source_component.h b/repos/base-hw/src/core/signal_source_component.h index 82186608f3..e4449fc3e7 100644 --- a/repos/base-hw/src/core/signal_source_component.h +++ b/repos/base-hw/src/core/signal_source_component.h @@ -19,7 +19,7 @@ /* core includes */ #include -#include +#include #include namespace Core {