From dbf812469861a85dee19f6752180705f466522ca Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Sun, 19 Jun 2022 11:03:51 +0000 Subject: [PATCH] Safely interpose fcntl() on architectures with bounded variadic arguments `fcntl()` abuses C variadic arguments to implement optional parameters. We want to forward this argument to the real `fcntl()` call, but doing so is non-portable. On most architectures, we can just read an `intptr_t`/`void*` and this will give us a (semi-)valid result even if no argument/an int was passed instead since `va_arg()` will generally read the next argument register or arbitrary data from the stack. On CHERI-enabled architectures, variadic arguments are tightly bounded, which means that reading a `void*` if an `int` was passed will result in a runtime trap. To avoid this problem this commit uses a new `vfcntl` libc function (https://github.com/CTSRD-CHERI/cheribsd/pull/2194), that takes a va_list instead of variadic arguments. Since this needs a new HAVE_VCNTL compile-time check, this adds a new config.h header that also includes the HAVE_TIMERFD value. This removes the need to pass -D flags on the command line. This reduces the number of test failures on CheriBSD Morello from 210 to 8. --- src/CMakeLists.txt | 9 +++++--- src/epoll_shim_config.h.cmake | 3 +++ src/epoll_shim_ctx.c | 23 +++++++++++-------- src/epoll_shim_ctx.h | 2 ++ src/epoll_shim_interpose.c | 12 ++-------- src/wrap.c | 43 ++++++++++++++++++++++++++++++++--- src/wrap.h | 2 ++ 7 files changed, 69 insertions(+), 25 deletions(-) create mode 100644 src/epoll_shim_config.h.cmake diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index e64a3a1..bd97ff6 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -25,6 +25,7 @@ add_library(wrap OBJECT wrap.c) set_property(TARGET wrap PROPERTY POSITION_INDEPENDENT_CODE ON) target_link_libraries(wrap PUBLIC Threads::Threads) target_include_directories(wrap + PRIVATE $ PUBLIC $) macro(add_compat_target _name _condition) @@ -51,6 +52,8 @@ include(CheckSymbolExists) # supports native timerfd descriptors. Prefer them if available. check_symbol_exists(eventfd "sys/eventfd.h" HAVE_EVENTFD) check_symbol_exists(timerfd_create "sys/timerfd.h" HAVE_TIMERFD) +# Presence of a variadic version of fcntl allows for safer interposing. +check_symbol_exists(vfcntl "fcntl.h;stdarg.h" HAVE_VFCNTL) check_symbol_exists(kqueue1 "sys/types.h;sys/event.h;sys/time.h" HAVE_KQUEUE1) add_compat_target(kqueue1 "NOT;HAVE_KQUEUE1") @@ -117,9 +120,9 @@ target_link_libraries( $ $ $) -if(HAVE_TIMERFD) - target_compile_definitions(epoll-shim PRIVATE HAVE_TIMERFD) -endif() +configure_file( + epoll_shim_config.h.cmake + ${CMAKE_CURRENT_BINARY_DIR}/epoll_shim_config.h) target_compile_definitions(epoll-shim PRIVATE EPOLL_SHIM_DISABLE_WRAPPER_MACROS) target_include_directories( epoll-shim diff --git a/src/epoll_shim_config.h.cmake b/src/epoll_shim_config.h.cmake new file mode 100644 index 0000000..057078f --- /dev/null +++ b/src/epoll_shim_config.h.cmake @@ -0,0 +1,3 @@ +#cmakedefine01 HAVE_TIMERFD +#cmakedefine01 HAVE_VFCNTL + diff --git a/src/epoll_shim_ctx.c b/src/epoll_shim_ctx.c index d0cec0d..7913715 100644 --- a/src/epoll_shim_ctx.c +++ b/src/epoll_shim_ctx.c @@ -20,6 +20,7 @@ #include #include +#include "epoll_shim_config.h" #include "epoll_shim_export.h" #include "errno_return.h" #include "timespec_util.h" @@ -427,7 +428,7 @@ epoll_shim_ctx_drop_desc(EpollShimCtx *epoll_shim_ctx, /**/ rwlock_unlock_write(&epoll_shim_ctx->rwlock); } -#ifndef HAVE_TIMERFD +#if !HAVE_TIMERFD static void trigger_realtime_change_notification(FileDescription *desc, int kq, void *arg) { @@ -792,30 +793,34 @@ epoll_shim_ppoll(struct pollfd *fds, nfds_t nfds, struct timespec const *tmo_p, EPOLL_SHIM_EXPORT int epoll_shim_fcntl(int fd, int cmd, ...) +{ + va_list ap; + va_start(ap, cmd); + int result = epoll_shim_vfcntl(fd, cmd, ap); + va_end(ap); + return result; +} + +EPOLL_SHIM_EXPORT +int +epoll_shim_vfcntl(int fd, int cmd, va_list ap) { ERRNO_SAVE; EpollShimCtx *epoll_shim_ctx; FileDescription *desc; - va_list ap; if (fd < 0 || (cmd != F_SETFL && cmd != F_GETFL) || epoll_shim_ctx_global(&epoll_shim_ctx) != 0 || (desc = epoll_shim_ctx_find_desc(epoll_shim_ctx, fd)) == NULL) { - va_start(ap, cmd); - void *arg = va_arg(ap, void *); - va_end(ap); - - ERRNO_RETURN(0, -1, real_fcntl(fd, cmd, arg)); + ERRNO_RETURN(0, -1, real_vfcntl(fd, cmd, ap)); } errno_t ec; int result = 0; if (cmd == F_SETFL) { - va_start(ap, cmd); int arg = va_arg(ap, int); - va_end(ap); (void)pthread_mutex_lock(&desc->mutex); { diff --git a/src/epoll_shim_ctx.h b/src/epoll_shim_ctx.h index 67719dc..edd69f4 100644 --- a/src/epoll_shim_ctx.h +++ b/src/epoll_shim_ctx.h @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -83,5 +84,6 @@ int epoll_shim_ppoll(struct pollfd *, nfds_t, struct timespec const *, sigset_t const *); int epoll_shim_fcntl(int fd, int cmd, ...); +int epoll_shim_vfcntl(int fd, int cmd, va_list); #endif diff --git a/src/epoll_shim_interpose.c b/src/epoll_shim_interpose.c index ba1a49e..83ce04c 100644 --- a/src/epoll_shim_interpose.c +++ b/src/epoll_shim_interpose.c @@ -6,14 +6,7 @@ #include #include "epoll_shim_interpose_export.h" - -int epoll_shim_close(int fd); -ssize_t epoll_shim_read(int fd, void *buf, size_t nbytes); -ssize_t epoll_shim_write(int fd, void const *buf, size_t nbytes); -int epoll_shim_poll(struct pollfd *, nfds_t, int); -int epoll_shim_ppoll(struct pollfd *, nfds_t, struct timespec const *, - sigset_t const *); -int epoll_shim_fcntl(int fd, int cmd, ...); +#include "epoll_shim_ctx.h" EPOLL_SHIM_INTERPOSE_EXPORT ssize_t @@ -65,8 +58,7 @@ fcntl(int fd, int cmd, ...) va_list ap; va_start(ap, cmd); - void *arg = va_arg(ap, void *); - int rv = epoll_shim_fcntl(fd, cmd, arg); + int rv = epoll_shim_vfcntl(fd, cmd, ap); va_end(ap); return rv; diff --git a/src/wrap.c b/src/wrap.c index f9c9d9a..ef5ffec 100644 --- a/src/wrap.c +++ b/src/wrap.c @@ -12,6 +12,7 @@ #include #include "compat_ppoll.h" +#include "epoll_shim_config.h" static struct { pthread_once_t wrap_init; @@ -27,7 +28,11 @@ static struct { typeof(ppoll) *real_ppoll; #endif #endif +#if !HAVE_VFCNTL typeof(fcntl) *real_fcntl; +#else + typeof(vfcntl) *real_vfcntl; +#endif } wrap = { .wrap_init = PTHREAD_ONCE_INIT }; static void @@ -65,7 +70,11 @@ wrap_initialize_impl(void) WRAP(ppoll); #endif #endif +#if !HAVE_VFCNTL WRAP(fcntl); +#else + WRAP(vfcntl); +#endif #undef WRAP } @@ -124,14 +133,42 @@ real_ppoll(struct pollfd fds[], nfds_t nfds, } int -real_fcntl(int fd, int cmd, ...) +real_vfcntl(int fd, int cmd, va_list ap) { wrap_initialize(); + + // fcntl() abuses C variadic arguments to implement optional parameters. + // We want to forward this argument to the real fcntl() call, but doing + // so is non-portable. On most architectures, we can just read an + // intptr_t/void* and this will give us a (semi-)valid result even if + // no argument/an int was passed instead since va_arg() will generally + // read the next argument register or arbitrary data from the stack. + // + // On CHERI-enabled architectures, variadic arguments are tightly + // bounded, which means that reading an 8-byte void* if a 4-byte int was + // passed will result in a runtime trap. Newer CheriBSD systems support + // a vfcntl() function that can be used to safely forward arguments to + // fcntl() without having to duplicate the logic to determine how many + // arguments will be read. +#if HAVE_VFCNTL + // vfcntl() is present, can just foward the va_list. + return wrap.real_vfcntl(fd, cmd, ap); +#else + // Otherwise, we assume that we can read a void* from the va_list + // even if the actual value passed was smaller or a different type and + // hope that this will match the calling convention. + void *arg = va_arg(ap, void *); + return wrap.real_fcntl(fd, cmd, arg); +#endif +} + +int +real_fcntl(int fd, int cmd, ...) +{ va_list ap; va_start(ap, cmd); - void *arg = va_arg(ap, void *); - int rv = wrap.real_fcntl(fd, cmd, arg); + int rv = real_vfcntl(fd, cmd, ap); va_end(ap); return rv; diff --git a/src/wrap.h b/src/wrap.h index 5910d45..9333615 100644 --- a/src/wrap.h +++ b/src/wrap.h @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -15,5 +16,6 @@ int real_ppoll(struct pollfd fds[], nfds_t nfds, struct timespec const *restrict timeout, sigset_t const *restrict newsigmask); int real_fcntl(int fd, int cmd, ...); +int real_vfcntl(int fd, int cmd, va_list ap); #endif