Adjust signal alternative stack size check #2153
Description
Hi, I found that in signal handling, Gramine needs to reserve memory for the xstate, sigframe, and restorer's address in the signal stack, but the user may not provide a large enough alternative stack, which will lead to undefined behavior and sometimes it is difficult to find the root cause. Therefore, perhaps we can add a patch in "libos_syscall_sigaltstack" to inform users?
void prepare_sigframe(PAL_CONTEXT* context, siginfo_t* siginfo, void* handler, void* restorer,
bool should_use_altstack, __sigset_t* old_mask) {
struct libos_thread* current = get_cur_thread();
uint64_t stack = get_stack_for_sighandler(context->rsp, should_use_altstack);
struct libos_xstate* xstate = NULL;
stack = ALIGN_DOWN(stack - libos_xstate_size(), alignof(*xstate));
xstate = (struct libos_xstate*)stack;
struct sigframe* sigframe = NULL;
stack = ALIGN_DOWN(stack - sizeof(*sigframe), alignof(*sigframe));
/* x64 SysV ABI requires that stack is aligned to 8 mod 0x10 after function call, so we have to
* mimic that in signal handler. `sigframe` will be aligned to 0x10 and we will push a return
* value (restorer address) on top of that later on. */
stack = ALIGN_DOWN(stack, 0x10);
/* Make sure `stack` is now aligned to both `alignof(*sigframe)` and 0x10. */
static_assert(alignof(*sigframe) % 0x10 == 0 || 0x10 % alignof(*sigframe) == 0,
"Incorrect sigframe alignment");
sigframe = (struct sigframe*)stack;
/* This could probably be omited as we set all fields explicitly below. */
memset(sigframe, 0, sizeof(*sigframe));
sigframe->siginfo = *siginfo;
/* Gramine does not change SS (stack segment register) and assumes that it is constant so
* these flags are not strictly needed, but we do store SS in ucontext so let's just set them. */
sigframe->uc.uc_flags = UC_SIGCONTEXT_SS | UC_STRICT_RESTORE_SS;
sigframe->uc.uc_link = NULL;
/* TODO: add support for SA_AUTODISARM
* Tracked in https://github.com/gramineproject/gramine/issues/84. */
sigframe->uc.uc_stack = current->signal_altstack;
pal_context_to_ucontext(&sigframe->uc, context);
/* XXX: Currently we assume that `struct libos_xstate`, `PAL_XREGS_STATE` and `struct _fpstate`
* (just the header) are the very same structure. This mess needs to be fixed. */
static_assert(sizeof(struct libos_xstate) == sizeof(PAL_XREGS_STATE),
"SSE state structs differ");
static_assert(sizeof(struct libos_fpstate) == sizeof(struct _fpstate),
"SSE state structs differ");
if (libos_xstate_copy(xstate, (struct libos_xstate*)sigframe->uc.uc_mcontext.fpstate)) {
/* This is a xsave-made xstate - it has the extended state info. */
sigframe->uc.uc_flags |= UC_FP_XSTATE;
}
sigframe->uc.uc_mcontext.fpstate = (struct _fpstate*)xstate;
sigframe->uc.uc_sigmask = *old_mask;
/* We always set all 3 arguments, even if it is not `SA_SIGINFO` handler. We can and it is
* easier this way. */
context->rdi = (long)siginfo->si_signo;
context->rsi = (uint64_t)&sigframe->siginfo;
context->rdx = (uint64_t)&sigframe->uc;
stack -= 8;
*(uint64_t*)stack = (uint64_t)restorer;
context->rip = (uint64_t)handler;
context->rsp = stack;
/* x64 SysV ABI mandates that DF flag is cleared and states that rest of flags is *not*
* preserved across function calls, hence we just set flags to a default value (IF). */
context->efl = 0x202;
/* If handler was defined as variadic/without prototype it would expect the number of vector
* register arguments in `rax`. */
context->rax = 0;
log_debug("Created sigframe for sig: %d at %p (handler: %p, restorer: %p)",
siginfo->si_signo, sigframe, handler, restorer);
}My patch is below, but I've only tested it in my case, so not sure if this patch will work (I'm not that familiar with the details of Gramine).
diff --git a/libos/src/sys/libos_sigaction.c b/libos/src/sys/libos_sigaction.c
index a4ceedbf..b23197f1 100644
--- a/libos/src/sys/libos_sigaction.c
+++ b/libos/src/sys/libos_sigaction.c
@@ -10,7 +10,9 @@
*/
#include <limits.h>
+#include <stddef.h>
+#include "libos_context.h"
#include "libos_internal.h"
#include "libos_ipc.h"
#include "libos_lock.h"
@@ -21,7 +23,14 @@
#include "libos_utils.h"
#include "linux_abi/errors.h"
#include "linux_abi/signals.h"
+#include "log.h"
#include "pal.h"
+#include "ucontext.h"
+
+struct sigframe {
+ ucontext_t uc;
+ siginfo_t siginfo;
+};
long libos_syscall_rt_sigaction(int signum, const struct __kernel_sigaction* act,
struct __kernel_sigaction* oldact, size_t sigsetsize) {
@@ -169,7 +178,15 @@ long libos_syscall_sigaltstack(const stack_t* ss, stack_t* oss) {
memset(cur_ss, 0, sizeof(*cur_ss));
cur_ss->ss_flags = SS_DISABLE;
} else {
- if (ss->ss_size < MINSIGSTKSZ) {
+ size_t min_ss_size =
+ MAX((libos_xstate_size() + sizeof(struct sigframe) + 8 /* restorer address */),
+ (size_t)MINSIGSTKSZ);
+ if (ss->ss_size < min_ss_size) {
+ log_error(
+ "The alternate stack size should be at least %ld, to store xstate, sigframe, "
+ "and restorer's address (possibly larger for alignment reasons), which is "
+ "special in Gramine. However, the user specified a size of %ld.",
+ min_ss_size, ss->ss_size);
return -ENOMEM;
}