| Message ID | 20200429194420.21147-2-dbuono@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for SafeStack | expand |
On Wed, Apr 29, 2020 at 03:44:17PM -0400, Daniele Buono wrote: > diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h > index bd6b0468e1..2ffd75ddbe 100644 > --- a/include/qemu/coroutine_int.h > +++ b/include/qemu/coroutine_int.h > @@ -28,6 +28,12 @@ > #include "qemu/queue.h" > #include "qemu/coroutine.h" > > +#if defined(__has_feature) && __has_feature(safe_stack) > +#define CONFIG_SAFESTACK 1 Please perform this feature check in ./configure. That way CONFIG_SAFESTACK will be defined alongside all the other CONFIG_* values and be available to C and Makefiles via config-host.h and config-host.mak. > @@ -160,6 +169,19 @@ Coroutine *qemu_coroutine_new(void) > /* swapcontext() in, siglongjmp() back out */ > if (!sigsetjmp(old_env, 0)) { > start_switch_fiber(&fake_stack_save, co->stack, co->stack_size); > +#ifdef CONFIG_SAFESTACK > + /* > + * Before we swap the context, set the new unsafe stack > + * The unsafe stack grows just like the normal stack, so start from > + * the last usable location of the memory area. > + * NOTE: we don't have to re-set it afterwards because sigsetjmp was > + * called with the original usp. Since we are not coming back with a > + * swapcontext, but with a siglongjmp, when we are back here we > + * already have usp restored to the valid one for this function I don't understand this comment. __safestack_unsafe_stack_ptr is a thread-local variable, not a CPU register. How will siglongjmp() automatically restore it? > + */ > + void *usp = co->unsafe_stack + co->unsafe_stack_size; > + __safestack_unsafe_stack_ptr = usp; > +#endif > swapcontext(&old_uc, &uc); > } >
Hi Stefan, thank you so much for reviewing this. See my answers below: On 5/21/2020 5:44 AM, Stefan Hajnoczi wrote: > On Wed, Apr 29, 2020 at 03:44:17PM -0400, Daniele Buono wrote: >> diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h >> index bd6b0468e1..2ffd75ddbe 100644 >> --- a/include/qemu/coroutine_int.h >> +++ b/include/qemu/coroutine_int.h >> @@ -28,6 +28,12 @@ >> #include "qemu/queue.h" >> #include "qemu/coroutine.h" >> >> +#if defined(__has_feature) && __has_feature(safe_stack) >> +#define CONFIG_SAFESTACK 1 > > Please perform this feature check in ./configure. That way > CONFIG_SAFESTACK will be defined alongside all the other CONFIG_* values > and be available to C and Makefiles via config-host.h and > config-host.mak. > Sure, will do this in v2. >> @@ -160,6 +169,19 @@ Coroutine *qemu_coroutine_new(void) >> /* swapcontext() in, siglongjmp() back out */ >> if (!sigsetjmp(old_env, 0)) { >> start_switch_fiber(&fake_stack_save, co->stack, co->stack_size); >> +#ifdef CONFIG_SAFESTACK >> + /* >> + * Before we swap the context, set the new unsafe stack >> + * The unsafe stack grows just like the normal stack, so start from >> + * the last usable location of the memory area. >> + * NOTE: we don't have to re-set it afterwards because sigsetjmp was >> + * called with the original usp. Since we are not coming back with a >> + * swapcontext, but with a siglongjmp, when we are back here we >> + * already have usp restored to the valid one for this function > > I don't understand this comment. __safestack_unsafe_stack_ptr is a > thread-local variable, not a CPU register. How will siglongjmp() > automatically restore it? > Correct, setjmp/longjmp have no visibility of the unsafe stack. What I meant is that it is not automatically restored by the longjmp itself, but by code the compiler adds around the sigsetjmp. Specifically, every sigsetjmp/sigjmp is intercepted by the compiler, the current value of __safestack_unsafe_stack_ptr is saved on the normal (safe) stack. Right after the sigsetjmp call it is then restored. I will change the comment to make it clearer. In practice, this is what happens: Original clang implementation in qemu_coroutine_new: ---- 40130c: callq 4008d0 <__sigsetjmp@plt> 401311: cmp $0x0,%eax 401314: jne 40132d <qemu_coroutine_new+0x12d> ---- Clang Implementation with safestack: ---- 4027a7: mov %rdx,-0x38(%rbp) <- Save unsafe ptr onto the safe stack [...] 40289c: callq 401410 <__sigsetjmp@plt> 4028a1: mov 0x201738(%rip),%rdi # 603fe0 <__safestack_unsafe_stack_ptr@@Base+0x603fe0> 4028a8: mov -0x38(%rbp),%rbx 4028ac: mov %rbx,%fs:(%rdi) <- Restore the unsafe ptr 4028b0: cmp $0x0,%eax 4028b3: jne 4028d9 <qemu_coroutine_new+0x179> >> + */ >> + void *usp = co->unsafe_stack + co->unsafe_stack_size; >> + __safestack_unsafe_stack_ptr = usp; >> +#endif >> swapcontext(&old_uc, &uc); >> } >>
On Fri, May 22, 2020 at 11:18:20AM -0400, Daniele Buono wrote: > On 5/21/2020 5:44 AM, Stefan Hajnoczi wrote: > > On Wed, Apr 29, 2020 at 03:44:17PM -0400, Daniele Buono wrote: > > > @@ -160,6 +169,19 @@ Coroutine *qemu_coroutine_new(void) > > > /* swapcontext() in, siglongjmp() back out */ > > > if (!sigsetjmp(old_env, 0)) { > > > start_switch_fiber(&fake_stack_save, co->stack, co->stack_size); > > > +#ifdef CONFIG_SAFESTACK > > > + /* > > > + * Before we swap the context, set the new unsafe stack > > > + * The unsafe stack grows just like the normal stack, so start from > > > + * the last usable location of the memory area. > > > + * NOTE: we don't have to re-set it afterwards because sigsetjmp was > > > + * called with the original usp. Since we are not coming back with a > > > + * swapcontext, but with a siglongjmp, when we are back here we > > > + * already have usp restored to the valid one for this function > > > > I don't understand this comment. __safestack_unsafe_stack_ptr is a > > thread-local variable, not a CPU register. How will siglongjmp() > > automatically restore it? > > > Correct, setjmp/longjmp have no visibility of the unsafe stack. What I > meant is that it is not automatically restored by the longjmp itself, > but by code the compiler adds around the sigsetjmp. > > Specifically, every sigsetjmp/sigjmp is intercepted by the compiler, the > current value of __safestack_unsafe_stack_ptr is saved on the normal > (safe) stack. > Right after the sigsetjmp call it is then restored. > > I will change the comment to make it clearer. > > In practice, this is what happens: > > Original clang implementation in qemu_coroutine_new: > ---- > 40130c: callq 4008d0 <__sigsetjmp@plt> > 401311: cmp $0x0,%eax > 401314: jne 40132d <qemu_coroutine_new+0x12d> > ---- > Clang Implementation with safestack: > ---- > 4027a7: mov %rdx,-0x38(%rbp) <- Save unsafe ptr onto the safe stack > [...] > 40289c: callq 401410 <__sigsetjmp@plt> > 4028a1: mov 0x201738(%rip),%rdi # 603fe0 > <__safestack_unsafe_stack_ptr@@Base+0x603fe0> > 4028a8: mov -0x38(%rbp),%rbx > 4028ac: mov %rbx,%fs:(%rdi) <- Restore the unsafe ptr > 4028b0: cmp $0x0,%eax > 4028b3: jne 4028d9 <qemu_coroutine_new+0x179> Oh, that's interesting. Thanks for explaining and updating the comment. Stefan
diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h index bd6b0468e1..2ffd75ddbe 100644 --- a/include/qemu/coroutine_int.h +++ b/include/qemu/coroutine_int.h @@ -28,6 +28,12 @@ #include "qemu/queue.h" #include "qemu/coroutine.h" +#if defined(__has_feature) && __has_feature(safe_stack) +#define CONFIG_SAFESTACK 1 +/* Pointer to the unsafe stack, defined by the compiler */ +extern __thread void *__safestack_unsafe_stack_ptr; +#endif + #define COROUTINE_STACK_SIZE (1 << 20) typedef enum { diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c index bd593e61bc..b79e9df9eb 100644 --- a/util/coroutine-ucontext.c +++ b/util/coroutine-ucontext.c @@ -41,6 +41,11 @@ typedef struct { Coroutine base; void *stack; size_t stack_size; +#ifdef CONFIG_SAFESTACK + /* Need an unsafe stack for each coroutine */ + void *unsafe_stack; + size_t unsafe_stack_size; +#endif sigjmp_buf env; #ifdef CONFIG_VALGRIND_H @@ -140,6 +145,10 @@ Coroutine *qemu_coroutine_new(void) co = g_malloc0(sizeof(*co)); co->stack_size = COROUTINE_STACK_SIZE; co->stack = qemu_alloc_stack(&co->stack_size); +#ifdef CONFIG_SAFESTACK + co->unsafe_stack_size = COROUTINE_STACK_SIZE; + co->unsafe_stack = qemu_alloc_stack(&co->unsafe_stack_size); +#endif co->base.entry_arg = &old_env; /* stash away our jmp_buf */ uc.uc_link = &old_uc; @@ -160,6 +169,19 @@ Coroutine *qemu_coroutine_new(void) /* swapcontext() in, siglongjmp() back out */ if (!sigsetjmp(old_env, 0)) { start_switch_fiber(&fake_stack_save, co->stack, co->stack_size); +#ifdef CONFIG_SAFESTACK + /* + * Before we swap the context, set the new unsafe stack + * The unsafe stack grows just like the normal stack, so start from + * the last usable location of the memory area. + * NOTE: we don't have to re-set it afterwards because sigsetjmp was + * called with the original usp. Since we are not coming back with a + * swapcontext, but with a siglongjmp, when we are back here we + * already have usp restored to the valid one for this function + */ + void *usp = co->unsafe_stack + co->unsafe_stack_size; + __safestack_unsafe_stack_ptr = usp; +#endif swapcontext(&old_uc, &uc); } @@ -192,6 +214,9 @@ void qemu_coroutine_delete(Coroutine *co_) #endif qemu_free_stack(co->stack, co->stack_size); +#ifdef CONFIG_SAFESTACK + qemu_free_stack(co->unsafe_stack, co->unsafe_stack_size); +#endif g_free(co); }
LLVM's SafeStack instrumentation does not yet support programs that make use of the APIs in ucontext.h With the current implementation of coroutine-ucontext, the resulting binary is incorrect, with different coroutines sharing the same unsafe stack and producing undefined behavior at runtime. This fix allocates an additional unsafe stack area for each coroutine, and sets the new unsafe stack pointer before calling swapcontext() in qemu_coroutine_new. This is the only place where the pointer needs to be manually updated, since sigsetjmp/siglongjmp are already instrumented by LLVM to properly support SafeStack. The additional stack is then freed in qemu_coroutine_delete. Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com> --- include/qemu/coroutine_int.h | 6 ++++++ util/coroutine-ucontext.c | 25 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+)