Re: Fiasco.OC: null-pointer dereference?
Matthias Lange <[email protected]>
| Newsgroups | gmane.comp.micro-kernel.l4.devel |
|---|---|
| Message-ID | <[email protected]> |
Hi Stefan, On 05/08/2017 03:36 PM, Matthias Lange wrote: > Hi Stefan, > > On 05/08/2017 09:09 AM, Stefan Kalkowski wrote: >> Dear L4-Hackers, >> >> recently, I started to upgrade the Fiasco.OC kernel version that is used >> by the Genode OS framework to the lastly released version (r72). I took >> the opportunity to upgrade, because the upcoming Genode release uses a >> fresh compiler toolchain that refused to build the very old Fiasco.OC >> kernel version that was used until now (r56). >> Everything went quite smoothly, and I'm glad to see how the kernel >> develops further. Thanks to all developers at this point! >> >> Unfortunately, I stumbled across an issue when it comes to thread >> destruction. In our system all threads are constructed and destructed by >> the roottask that is called 'core'. In some cases, not always but quite >> often, the Ram_quota pointer of the thread object is zero during the >> call of the Thread_oject's delete operator, which leads to a page-fault >> within the kernel-code. A simple check[1] before dereferencing the >> pointer solves the problem, but I wonder whether we will leak quota or >> memory then, or in general cover some more serious problem. > > Thank you for reporting this issue. I will forward this to our kernel > maintainer. > > Could you elaborate a little bit more on the circumstances leading to > this issue? I wonder whether we can come up with a simple test case > triggering the page fault. No need to come up with a test case. It turns out that your problem originates in an unfortunate combination of "old" sources and new toolchain. C++ allows the compiler to elide writes to objects that are later intialized by a constructor which leads to the _quota member not being initialized correctly under all circumstances. That also answers your inital question that, yes, your check covers a more serious problem :). Could you please try the attached patch? It should fix the problem. Best, Matthias. > > Best, > Matthias. > >> Obviously, we have different usage patterns of syscalls, e.g.: the order >> of destructing IPC-gates, threads, IRQs, and tasks. Moreover, we still >> have some very few patches[2] so that the kernel meets our requirements. >> But none of them explains the thread's Ram_quota pointer getting zero. >> The page-fault triggers across all x86 and arm platforms that we use. >> >> Any hint would be very much appreciated, all the best! >> Stefan >> >> [1] >> https://github.com/skalk/foc/commit/2b01c9d16fd8e29e6af18fe750be2c8a312b4762 >> [2] https://github.com/skalk/foc/commits/r72 _______________________________________________ l4-hackers mailing list [email protected] http://os.inf.tu-dresden.de/mailman/listinfo/l4-hackers
0001-Fix-invalid-initialization-in-new.patch
(text/x-patch, 11.3 KB)
From d8daf7077abb9290cf1603fc73e12db618f92b12 Mon Sep 17 00:00:00 2001 From: Alexander Warg <[email protected]> Date: Wed, 5 Oct 2016 15:20:59 +0200 Subject: [PATCH] Fix invalid initialization in 'new' C++ allows the compiler to elide write to objects that are later initialized with a constructor. This may lead to the situation that the `_quota` member of a thread is not correctly initialized. To fix this we need to pass the correct quota object to each Thread constructor instead. Change-Id: Iac0ad2963b86f8393df6ad0c18adde386d9b1179 --- src/kern/app_cpu_thread.cpp | 7 ++++++- src/kern/arm/main.cpp | 2 +- src/kern/arm/thread-arm.cpp | 11 ++++++----- src/kern/ia32/32/main-ia32-32.cpp | 2 +- src/kern/ia32/64/main-ia32-64.cpp | 2 +- src/kern/ia32/thread-ia32.cpp | 3 ++- src/kern/kernel_thread-std.cpp | 4 ++-- src/kern/kernel_thread.cpp | 5 +++-- src/kern/mips/main.cpp | 2 +- src/kern/mips/thread-mips.cpp | 5 +++-- src/kern/ppc32/main.cpp | 2 +- src/kern/ppc32/thread-ppc32.cpp | 11 ++++++----- src/kern/sparc/main.cpp | 2 +- src/kern/sparc/thread-sparc.cpp | 5 +++-- src/kern/thread.cpp | 14 ++++++-------- src/kern/thread_object.cpp | 11 ++++++----- 16 files changed, 49 insertions(+), 39 deletions(-) diff --git a/src/kern/app_cpu_thread.cpp b/src/kern/app_cpu_thread.cpp index 8717355..57d2d9d 100644 --- a/src/kern/app_cpu_thread.cpp +++ b/src/kern/app_cpu_thread.cpp @@ -28,6 +28,11 @@ IMPLEMENTATION [mp]: #include "timer_tick.h" #include "spin_lock.h" +PUBLIC explicit inline +App_cpu_thread::App_cpu_thread(Ram_quota *q) +: Kernel_thread(q) +{} + PUBLIC static Kernel_thread * App_cpu_thread::may_be_create(Cpu_number cpu, bool cpu_never_seen_before) @@ -38,7 +43,7 @@ App_cpu_thread::may_be_create(Cpu_number cpu, bool cpu_never_seen_before) return static_cast<Kernel_thread *>(kernel_context(cpu)); } - Kernel_thread *t = new (Ram_quota::root) App_cpu_thread; + Kernel_thread *t = new (Ram_quota::root) App_cpu_thread(Ram_quota::root); assert (t); t->set_home_cpu(cpu); diff --git a/src/kern/arm/main.cpp b/src/kern/arm/main.cpp index c44c154..379f300 100644 --- a/src/kern/arm/main.cpp +++ b/src/kern/arm/main.cpp @@ -82,7 +82,7 @@ kernel_main() // pic_disable_all(); // create kernel thread - static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread; + static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread(Ram_quota::root); Task *const ktask = Kernel_task::kernel_task(); check(kernel->bind(ktask, User<Utcb>::Ptr(0))); assert(((Mword)kernel->init_stack() & 7) == 0); diff --git a/src/kern/arm/thread-arm.cpp b/src/kern/arm/thread-arm.cpp index 727da91..632621e 100644 --- a/src/kern/arm/thread-arm.cpp +++ b/src/kern/arm/thread-arm.cpp @@ -330,11 +330,12 @@ IMPLEMENTATION [arm]: @post state() != 0 */ IMPLEMENT -Thread::Thread() - : Sender(0), // select optimized version of constructor - _pager(Thread_ptr::Invalid), - _exc_handler(Thread_ptr::Invalid), - _del_observer(0) +Thread::Thread(Ram_quota *q) +: Sender(0), + _pager(Thread_ptr::Invalid), + _exc_handler(Thread_ptr::Invalid), + _quota(q), + _del_observer(0) { assert (state(false) == 0); diff --git a/src/kern/ia32/32/main-ia32-32.cpp b/src/kern/ia32/32/main-ia32-32.cpp index 9f0ce7f..c1a73a3 100644 --- a/src/kern/ia32/32/main-ia32-32.cpp +++ b/src/kern/ia32/32/main-ia32-32.cpp @@ -45,7 +45,7 @@ kernel_main(void) main_arch(); // create kernel thread - static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread; + static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread(Ram_quota::root); assert_opt (kernel); Task *const ktask = Kernel_task::kernel_task(); check(kernel->bind(ktask, User<Utcb>::Ptr(0))); diff --git a/src/kern/ia32/64/main-ia32-64.cpp b/src/kern/ia32/64/main-ia32-64.cpp index 4d7aebd..3857361 100644 --- a/src/kern/ia32/64/main-ia32-64.cpp +++ b/src/kern/ia32/64/main-ia32-64.cpp @@ -42,7 +42,7 @@ kernel_main(void) main_arch(); // create kernel thread - static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread; + static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread(Ram_quota::root); Task *const ktask = Kernel_task::kernel_task(); check(kernel->bind(ktask, User<Utcb>::Ptr(0))); diff --git a/src/kern/ia32/thread-ia32.cpp b/src/kern/ia32/thread-ia32.cpp index 6ee6534..47f8365 100644 --- a/src/kern/ia32/thread-ia32.cpp +++ b/src/kern/ia32/thread-ia32.cpp @@ -55,11 +55,12 @@ IMPLEMENTATION [ia32,amd64,ux]: Trap_state::Handler Thread::nested_trap_handler FIASCO_FASTCALL; IMPLEMENT -Thread::Thread() +Thread::Thread(Ram_quota *q) : Receiver(), Sender(0), // select optimized version of constructor _pager(Thread_ptr::Invalid), _exc_handler(Thread_ptr::Invalid), + _quota(q), _del_observer(0) { assert (state(false) == 0); diff --git a/src/kern/kernel_thread-std.cpp b/src/kern/kernel_thread-std.cpp index 55370e6..fdf4451 100644 --- a/src/kern/kernel_thread-std.cpp +++ b/src/kern/kernel_thread-std.cpp @@ -73,7 +73,7 @@ Kernel_thread::init_workload() check(map(o, sigma0, sigma0, c, 0)); } - Thread_object *sigma0_thread = new (Ram_quota::root) Thread_object(); + Thread_object *sigma0_thread = new (Ram_quota::root) Thread_object(Ram_quota::root); assert(sigma0_thread); @@ -99,7 +99,7 @@ Kernel_thread::init_workload() // prevent deletion of this thing boot_task->inc_ref(); - Thread_object *boot_thread = new (Ram_quota::root) Thread_object(); + Thread_object *boot_thread = new (Ram_quota::root) Thread_object(Ram_quota::root); assert (boot_thread); diff --git a/src/kern/kernel_thread.cpp b/src/kern/kernel_thread.cpp index 8b4cfe5..348fde9 100644 --- a/src/kern/kernel_thread.cpp +++ b/src/kern/kernel_thread.cpp @@ -45,8 +45,9 @@ IMPLEMENTATION: #include "watchdog.h" -PUBLIC -Kernel_thread::Kernel_thread() : Thread_object(Thread::Kernel) +PUBLIC explicit +Kernel_thread::Kernel_thread(Ram_quota *q) +: Thread_object(q, Thread::Kernel) {} PUBLIC inline diff --git a/src/kern/mips/main.cpp b/src/kern/mips/main.cpp index b78ed4f..163059e 100644 --- a/src/kern/mips/main.cpp +++ b/src/kern/mips/main.cpp @@ -78,7 +78,7 @@ extern "C" void kernel_main() set_exit_question(&exit_question); // create kernel thread - static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread; + static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread(Ram_quota::root); Task *const ktask = Kernel_task::kernel_task(); check(kernel->bind(ktask, User<Utcb>::Ptr(0))); assert(((Mword)kernel->init_stack() & 7) == 0); diff --git a/src/kern/mips/thread-mips.cpp b/src/kern/mips/thread-mips.cpp index 34a5af3..f5cbc89 100644 --- a/src/kern/mips/thread-mips.cpp +++ b/src/kern/mips/thread-mips.cpp @@ -99,10 +99,11 @@ IMPLEMENT inline void Thread::user_ip(Mword ip) { regs()->ip(ip); } @post state() != 0 */ IMPLEMENT -Thread::Thread() -: Sender (0), // select optimized version of constructor +Thread::Thread(Ram_quota *q) +: Sender(0), _pager(Thread_ptr::Invalid), _exc_handler(Thread_ptr::Invalid), + _quota(q), _del_observer(0) { diff --git a/src/kern/ppc32/main.cpp b/src/kern/ppc32/main.cpp index 6bc721a..75db154 100644 --- a/src/kern/ppc32/main.cpp +++ b/src/kern/ppc32/main.cpp @@ -82,7 +82,7 @@ int main() // pic_disable_all(); // create kernel thread - static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread; + static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread(Ram_quota::root); Task *const ktask = Kernel_task::kernel_task(); check(kernel->bind(ktask, User<Utcb>::Ptr(0))); //kdb_ke("init"); diff --git a/src/kern/ppc32/thread-ppc32.cpp b/src/kern/ppc32/thread-ppc32.cpp index 82ea299..b35812b 100644 --- a/src/kern/ppc32/thread-ppc32.cpp +++ b/src/kern/ppc32/thread-ppc32.cpp @@ -239,11 +239,12 @@ IMPLEMENTATION [ppc32]: @post state() != 0 */ IMPLEMENT -Thread::Thread() - : Sender (0), // select optimized version of constructor - _pager(Thread_ptr::Invalid), - _exc_handler(Thread_ptr::Invalid), - _del_observer(0) +Thread::Thread(Ram_quota *q) +: Sender(0), + _pager(Thread_ptr::Invalid), + _exc_handler(Thread_ptr::Invalid), + _quota(q), + _del_observer(0) { assert(state(false) == 0); diff --git a/src/kern/sparc/main.cpp b/src/kern/sparc/main.cpp index fe77fb8..17ff28d 100644 --- a/src/kern/sparc/main.cpp +++ b/src/kern/sparc/main.cpp @@ -81,7 +81,7 @@ int main() // pic_disable_all(); // create kernel thread - static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread; + static Kernel_thread *kernel = new (Ram_quota::root) Kernel_thread(Ram_quota::root); Task *const ktask = Kernel_task::kernel_task(); check(kernel->bind(ktask, User<Utcb>::Ptr(0))); //kdb_ke("init"); diff --git a/src/kern/sparc/thread-sparc.cpp b/src/kern/sparc/thread-sparc.cpp index 7123e18..0b0f6ab 100644 --- a/src/kern/sparc/thread-sparc.cpp +++ b/src/kern/sparc/thread-sparc.cpp @@ -164,10 +164,11 @@ IMPLEMENTATION [sparc]: @post state() != 0 */ IMPLEMENT -Thread::Thread() - : Sender (0), // select optimized version of constructor +Thread::Thread(Ram_quota *q) + : Sender(0), _pager(Thread_ptr::Invalid), _exc_handler(Thread_ptr::Invalid), + _quota(q), _del_observer(0) { diff --git a/src/kern/thread.cpp b/src/kern/thread.cpp index 53a883d..89929ba 100644 --- a/src/kern/thread.cpp +++ b/src/kern/thread.cpp @@ -96,7 +96,7 @@ public: * * @post state() != 0. */ - Thread(); + explicit Thread(Ram_quota *); int handle_page_fault(Address pfa, Mword error, Mword pc, Return_frame *regs); @@ -139,7 +139,7 @@ public: bool arch_ext_vcpu_enabled(); protected: - explicit Thread(Context_mode_kernel); + explicit Thread(Ram_quota *, Context_mode_kernel); // More ipc state Thread_ptr _pager; @@ -198,10 +198,8 @@ Thread::operator new(size_t, Ram_quota *q) throw () { void *t = Kmem_alloc::allocator()->q_unaligned_alloc(q, Thread::Size); if (t) - { - memset(t, 0, sizeof(Thread)); - reinterpret_cast<Thread*>(t)->_quota = q; - } + memset(t, 0, sizeof(Thread)); + return t; } @@ -272,8 +270,8 @@ Thread::unbind() @param id user-visible thread ID of the sender */ IMPLEMENT inline -Thread::Thread(Context_mode_kernel) - : Receiver(), Sender(), _del_observer(0), _magic(magic) +Thread::Thread(Ram_quota *q, Context_mode_kernel) + : Receiver(), Sender(), _quota(q), _del_observer(0), _magic(magic) { inc_ref(); _space.space(Kernel_task::kernel_task()); diff --git a/src/kern/thread_object.cpp b/src/kern/thread_object.cpp index bafb6ad..0591542 100644 --- a/src/kern/thread_object.cpp +++ b/src/kern/thread_object.cpp @@ -68,11 +68,12 @@ Obj_cap::revalidate(Kobject_iface *o) return deref() == o; } -PUBLIC -Thread_object::Thread_object() : Thread() {} +PUBLIC explicit +Thread_object::Thread_object(Ram_quota *q) : Thread(q) {} -PUBLIC -Thread_object::Thread_object(Context_mode_kernel k) : Thread(k) {} +PUBLIC explicit +Thread_object::Thread_object(Ram_quota *q, Context_mode_kernel k) +: Thread(q, k) {} PUBLIC virtual bool @@ -693,7 +694,7 @@ thread_factory(Ram_quota *q, Space *, int *err) { *err = L4_err::ENomem; - return new (q) Thread_object(); + return new (q) Thread_object(q); } static inline void __attribute__((constructor)) FIASCO_INIT -- 2.7.4