From 1a6963813c9efca904db43d7a78654c11baa5152 Mon Sep 17 00:00:00 2001 From: Alexander Boettcher Date: Mon, 20 Feb 2017 13:30:36 +0100 Subject: [PATCH] libc: avoid race using Libc::suspend with pthreads TOCTTOU bug, in our case time of check to time of sleep bug --- repos/libports/src/lib/libc/nanosleep.cc | 3 +- repos/libports/src/lib/libc/select.cc | 16 ++++- .../libports/src/lib/libc/socket_fs_plugin.cc | 9 ++- repos/libports/src/lib/libc/task.cc | 31 ++++++---- repos/libports/src/lib/libc/task.h | 3 +- repos/libports/src/lib/libc/vfs_plugin.cc | 60 ++++++++++++++++--- 6 files changed, 98 insertions(+), 24 deletions(-) diff --git a/repos/libports/src/lib/libc/nanosleep.cc b/repos/libports/src/lib/libc/nanosleep.cc index dd3e98c8c2..c46a93a817 100644 --- a/repos/libports/src/lib/libc/nanosleep.cc +++ b/repos/libports/src/lib/libc/nanosleep.cc @@ -23,7 +23,8 @@ int _nanosleep(const struct timespec *req, struct timespec *rem) { unsigned long sleep_ms = req->tv_sec*1000 + req->tv_nsec/1000000; - do { sleep_ms = Libc::suspend(sleep_ms); } while (sleep_ms); + struct Check : Libc::Suspend_functor { bool suspend() override { return true; } } check; + do { sleep_ms = Libc::suspend(check, sleep_ms); } while (sleep_ms); if (rem) { rem->tv_sec = 0; diff --git a/repos/libports/src/lib/libc/select.cc b/repos/libports/src/lib/libc/select.cc index a8069df735..620451fcea 100644 --- a/repos/libports/src/lib/libc/select.cc +++ b/repos/libports/src/lib/libc/select.cc @@ -269,9 +269,19 @@ _select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, Timeout(timeval *tv) : _tv(tv) { } } timeout { tv }; - do { - timeout.duration = Libc::suspend(timeout.duration); - } while (!timeout.expired() && select_cb->nready == 0); + struct Check : Libc::Suspend_functor { + struct Timeout *timeout; + Libc::Select_cb *select_cb; + + Check(Timeout *timeout, Libc::Select_cb * select_cb) + : timeout(timeout), select_cb(select_cb) { } + + bool suspend() override { + return !timeout->expired() && select_cb->nready == 0; } + } check ( &timeout, &*select_cb ); + + while (!timeout.expired() && select_cb->nready == 0) + timeout.duration = Libc::suspend(check, timeout.duration); select_cb_list.remove(&(*select_cb)); diff --git a/repos/libports/src/lib/libc/socket_fs_plugin.cc b/repos/libports/src/lib/libc/socket_fs_plugin.cc index a27d637dcd..942f656d1b 100644 --- a/repos/libports/src/lib/libc/socket_fs_plugin.cc +++ b/repos/libports/src/lib/libc/socket_fs_plugin.cc @@ -283,8 +283,15 @@ static int read_sockaddr_in(int read_fd, Socket_context *context, if (!addrlen || *addrlen <= 0) return Errno(EINVAL); if (read_fd == -1) return Errno(ENOTCONN); + struct Check : Libc::Suspend_functor { + Socket_context *context; + Check (Socket_context *context) : context (context) { } + bool suspend() override { + return !context->remote_read_ready(); } + } check ( context ); + while (block_for_read && !context->remote_read_ready()) - Libc::suspend(); + Libc::suspend(check); Sockaddr_string addr_string; int const n = read(read_fd, addr_string.base(), addr_string.capacity() - 1); diff --git a/repos/libports/src/lib/libc/task.cc b/repos/libports/src/lib/libc/task.cc index f52d7871da..855ea3640c 100644 --- a/repos/libports/src/lib/libc/task.cc +++ b/repos/libports/src/lib/libc/task.cc @@ -280,7 +280,7 @@ struct Libc::Pthreads p->lock.unlock(); } - unsigned long suspend_myself(unsigned long timeout_ms) + unsigned long suspend_myself(Suspend_functor & check, unsigned long timeout_ms) { Pthread myself { timer_accessor, timeout_ms }; { @@ -289,7 +289,10 @@ struct Libc::Pthreads myself.next = pthreads; pthreads = &myself; } - myself.lock.lock(); + + if (check.suspend()) + myself.lock.lock(); + { Genode::Lock::Guard g(mutex); @@ -449,9 +452,13 @@ struct Libc::Kernel */ static void _user_entry(Libc::Kernel *kernel) { + struct Check : Suspend_functor { + bool suspend() override { return true; } + } check; + kernel->_app_code->execute(); kernel->_app_returned = true; - kernel->_suspend_main(0); + kernel->_suspend_main(check, 0); } bool _main_context() const { return &_myself == Genode::Thread::myself(); } @@ -484,8 +491,12 @@ struct Libc::Kernel _longjmp(_user_context, 1); } - unsigned long _suspend_main(unsigned long timeout_ms) + unsigned long _suspend_main(Suspend_functor &check, + unsigned long timeout_ms) { + if (!check.suspend()) + return 0; + if (timeout_ms > 0) _main_timeout.timeout(timeout_ms); @@ -562,7 +573,7 @@ struct Libc::Kernel /** * Suspend this context (main or pthread) */ - unsigned long suspend(unsigned long timeout_ms) + unsigned long suspend(Suspend_functor &check, unsigned long timeout_ms) { if (timeout_ms > 0 && timeout_ms > _timer_accessor.timer().max_timeout()) { @@ -573,8 +584,8 @@ struct Libc::Kernel timeout_ms = min(timeout_ms, _timer_accessor.timer().max_timeout()); } - return _main_context() ? _suspend_main(timeout_ms) - : _pthreads.suspend_myself(timeout_ms); + return _main_context() ? _suspend_main(check, timeout_ms) + : _pthreads.suspend_myself(check, timeout_ms); } unsigned long current_time() @@ -669,12 +680,12 @@ static void resumed_callback() { kernel->entrypoint_resumed(); } void Libc::resume_all() { kernel->resume_all(); } -unsigned long Libc::suspend(unsigned long timeout_ms) +unsigned long Libc::suspend(Suspend_functor &s, + unsigned long timeout_ms) { - return kernel->suspend(timeout_ms); + return kernel->suspend(s, timeout_ms); } - unsigned long Libc::current_time() { return kernel->current_time(); diff --git a/repos/libports/src/lib/libc/task.h b/repos/libports/src/lib/libc/task.h index 8ad368701e..c911dae585 100644 --- a/repos/libports/src/lib/libc/task.h +++ b/repos/libports/src/lib/libc/task.h @@ -47,7 +47,8 @@ namespace Libc { * or as separate pthread. This function returns after the libc kernel * resumed the user context execution. */ - unsigned long suspend(unsigned long timeout_ms = 0UL); + struct Suspend_functor { virtual bool suspend() = 0; }; + unsigned long suspend(Suspend_functor &, unsigned long timeout_ms = 0UL); /** * Get time since startup in ms diff --git a/repos/libports/src/lib/libc/vfs_plugin.cc b/repos/libports/src/lib/libc/vfs_plugin.cc index 6aaf77853b..ca582a6d43 100644 --- a/repos/libports/src/lib/libc/vfs_plugin.cc +++ b/repos/libports/src/lib/libc/vfs_plugin.cc @@ -290,24 +290,61 @@ ssize_t Libc::Vfs_plugin::write(Libc::File_descriptor *fd, const void *buf, } +typedef Vfs::File_io_service::Read_result Result; + +struct Read_check : Libc::Suspend_functor { + Vfs::Vfs_handle * handle; + void * buf; + ::size_t * count; + Vfs::file_size * out_count; + Result * out_result; + + Read_check(Vfs::Vfs_handle * handle, void * buf, ::size_t * count, + Vfs::file_size * out_count, Result * out_result) + : handle(handle), buf(buf), count(count), out_count(out_count), + out_result(out_result) + { } +}; + ssize_t Libc::Vfs_plugin::read(Libc::File_descriptor *fd, void *buf, ::size_t count) { - typedef Vfs::File_io_service::Read_result Result; - Vfs::Vfs_handle *handle = vfs_handle(fd); Vfs::file_size out_count = 0; Result out_result = Result::READ_OK; while (!handle->fs().queue_read(handle, (char *)buf, count, - out_result, out_count)) - Libc::suspend(); + out_result, out_count)) { + struct Check : Read_check { + Check(Vfs::Vfs_handle * handle, void * buf, ::size_t * count, + Vfs::file_size * out_count, Result * out_result) + : Read_check (handle, buf, count, out_count, out_result) { } + + bool suspend() override { + return !handle->fs().queue_read(handle, (char *)buf, *count, + *out_result, *out_count); } + } check ( handle, buf, &count, &out_count, &out_result); + + Libc::suspend(check); + } while (out_result == Result::READ_QUEUED) { - out_result = handle->fs().complete_read(handle, (char *)buf, count, out_count); - if (out_result == Result::READ_QUEUED) - Libc::suspend(); + + struct Check : Read_check { + Check(Vfs::Vfs_handle * handle, void * buf, ::size_t * count, + Vfs::file_size * out_count, Result * out_result) + : Read_check (handle, buf, count, out_count, out_result) { } + + bool suspend() override { + *out_result = handle->fs().complete_read(handle, (char *)buf, + *count, *out_count); + /* suspend me if read is still queued */ + return *out_result == Result::READ_QUEUED; + } + } check ( handle, buf, &count, &out_count, &out_result); + + Libc::suspend(check); } switch (out_result) { @@ -788,8 +825,15 @@ int Libc::Vfs_plugin::select(int nfds, FD_SET(fd, readfds); ++nready; } else { + struct Check : Libc::Suspend_functor { + Vfs::Vfs_handle * handle; + Check(Vfs::Vfs_handle * handle) : handle (handle) { } + bool suspend() override { + return !handle->fs().notify_read_ready(handle); } + } check ( handle ); + while (!handle->fs().notify_read_ready(handle)) - Libc::suspend(); + Libc::suspend(check); } }