[PATCH 0/4] Add support for SafeStack

[PATCH 0/4] Add support for SafeStack

[Daniele Buono]

LLVM supports SafeStack instrumentation to protect against stack buffer
overflows, since version 3.7

From https://clang.llvm.org/docs/SafeStack.html:
"It works by separating the program stack into two distinct regions: the
safe stack and the unsafe stack. The safe stack stores return addresses,
register spills, and local variables that are always accessed in a safe
way, while the unsafe stack stores everything else. This separation
ensures that buffer overflows on the unsafe stack cannot be used to
overwrite anything on the safe stack."

Unfortunately, the use of two stack regions does not cope well with
QEMU's coroutines. The second stack region is not properly set up with
both ucontext and sigaltstack, so multiple coroutines end up sharing the
same memory area for the unsafe stack, causing undefined behaviors at
runtime (and most iochecks to fail).

This patch series fixes the implementation of the ucontext backend and
make sure that sigaltstack is never used if the compiler is applying
the SafeStack instrumentation. It also adds a configure flag to enable
SafeStack, and enables iotests when SafeStack is used.

This is an RFC mainly because of the low-level use of the SafeStack
runtime.
When running swapcontext(), we have to manually set the unsafe stack
pointer to the new area allocated for the coroutine. LLVM does not allow
this by using builtin, so we have to use implementation details that may
change in the future.
This patch has been tested briefly ( make check on an x86 system ) with
clang v3.9, v4.0, v5.0, v6.0
Heavier testing, with make check-acceptance has been performed with
clang v7.0

Daniele Buono (4):
  coroutine: support SafeStack in ucontext backend
  coroutine: Add check for SafeStack in sigalstack
  configure: add flag to enable SafeStack
  check-block: Enable iotests with SafeStack

 configure                    | 29 +++++++++++++++++++++++++++++
 include/qemu/coroutine_int.h |  6 ++++++
 tests/check-block.sh         | 12 +++++++++++-
 util/coroutine-sigaltstack.c |  4 ++++
 util/coroutine-ucontext.c    | 25 +++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 1 deletion(-)

-- 
2.26.2

[PATCH 1/4] coroutine: support SafeStack in ucontext backend

[Daniele Buono]

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(+)

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);
 }
 
-- 
2.26.2

Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]

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);
>      }
>  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend

[Daniele Buono]

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);
>>       }
>>   

Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/4] coroutine: Add check for SafeStack in sigalstack

[Daniele Buono]

LLVM's SafeStack instrumentation cannot be used inside signal handlers
that make use of sigaltstack().
Since coroutine-sigaltstack relies on sigaltstack(), it is not
compatible with SafeStack. The resulting binary is incorrect, with
different coroutines sharing the same unsafe stack and producing
undefined behavior at runtime.
To avoid this, we add a check in coroutine-sigaltstack that throws a
preprocessor #error and interrupt the compilation if SafeStack is
enabled.

Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
---
 util/coroutine-sigaltstack.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index f6fc49a0e5..b7cdc959f8 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -30,6 +30,10 @@
 #include "qemu-common.h"
 #include "qemu/coroutine_int.h"
 
+#ifdef CONFIG_SAFESTACK
+#error "SafeStack does not work with sigaltstack's implementation"
+#endif
+
 typedef struct {
     Coroutine base;
     void *stack;
-- 
2.26.2

Re: [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack

[Philippe Mathieu-Daudé]

On 4/29/20 9:44 PM, Daniele Buono wrote:
> LLVM's SafeStack instrumentation cannot be used inside signal handlers
> that make use of sigaltstack().
> Since coroutine-sigaltstack relies on sigaltstack(), it is not
> compatible with SafeStack. The resulting binary is incorrect, with
> different coroutines sharing the same unsafe stack and producing
> undefined behavior at runtime.
> To avoid this, we add a check in coroutine-sigaltstack that throws a
> preprocessor #error and interrupt the compilation if SafeStack is
> enabled.
> 
> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> ---
>   util/coroutine-sigaltstack.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
> index f6fc49a0e5..b7cdc959f8 100644
> --- a/util/coroutine-sigaltstack.c
> +++ b/util/coroutine-sigaltstack.c
> @@ -30,6 +30,10 @@
>   #include "qemu-common.h"
>   #include "qemu/coroutine_int.h"
>   
> +#ifdef CONFIG_SAFESTACK
> +#error "SafeStack does not work with sigaltstack's implementation"
> +#endif
> +
>   typedef struct {
>       Coroutine base;
>       void *stack;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

On Wed, Apr 29, 2020 at 03:44:18PM -0400, Daniele Buono wrote:

s/sigalstack/sigaltstack/ in the commit message.

> LLVM's SafeStack instrumentation cannot be used inside signal handlers
> that make use of sigaltstack().
> Since coroutine-sigaltstack relies on sigaltstack(), it is not
> compatible with SafeStack. The resulting binary is incorrect, with
> different coroutines sharing the same unsafe stack and producing
> undefined behavior at runtime.
> To avoid this, we add a check in coroutine-sigaltstack that throws a
> preprocessor #error and interrupt the compilation if SafeStack is
> enabled.
> 
> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> ---
>  util/coroutine-sigaltstack.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
> index f6fc49a0e5..b7cdc959f8 100644
> --- a/util/coroutine-sigaltstack.c
> +++ b/util/coroutine-sigaltstack.c
> @@ -30,6 +30,10 @@
>  #include "qemu-common.h"
>  #include "qemu/coroutine_int.h"
>  
> +#ifdef CONFIG_SAFESTACK
> +#error "SafeStack does not work with sigaltstack's implementation"
> +#endif

Neither the commit description nor the #error message explain why it
doesn't work. Could it work in the future or is there a fundamental
reason why it will never work?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack

[Daniele Buono]

Sorry, missed the question at the end of the email.
Will change the commit and error message to explain better in v2.

Similar to the ucontext, case, sigaltstack does not work out of the box
because it requires a stack to be allocated by the user.

I'll be honest, I didn't check the details of how sigaltstack is used in
coroutine-sigaltstack. It is very possible that this coroutine version
can also be adapted to run properly with SafeStack.
coroutine_trampoline is probably the place where we can set the usp so
that, when coroutine_bootstrap is called, it is done with the new unsafe
stack.

My initial focus was on the ucontext implementation since that is the
default one and mostly used. For now, I am just blocking the user from
using coroutine-sigaltstack with SafeStack.

However, if you are interested, Ican try to add support to sigaltstack
in the future.

On 5/21/2020 5:49 AM, Stefan Hajnoczi wrote:
> On Wed, Apr 29, 2020 at 03:44:18PM -0400, Daniele Buono wrote:
> 
> s/sigalstack/sigaltstack/ in the commit message.
> 
>> LLVM's SafeStack instrumentation cannot be used inside signal handlers
>> that make use of sigaltstack().
>> Since coroutine-sigaltstack relies on sigaltstack(), it is not
>> compatible with SafeStack. The resulting binary is incorrect, with
>> different coroutines sharing the same unsafe stack and producing
>> undefined behavior at runtime.
>> To avoid this, we add a check in coroutine-sigaltstack that throws a
>> preprocessor #error and interrupt the compilation if SafeStack is
>> enabled.
>>
>> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
>> ---
>>   util/coroutine-sigaltstack.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
>> index f6fc49a0e5..b7cdc959f8 100644
>> --- a/util/coroutine-sigaltstack.c
>> +++ b/util/coroutine-sigaltstack.c
>> @@ -30,6 +30,10 @@
>>   #include "qemu-common.h"
>>   #include "qemu/coroutine_int.h"
>>   
>> +#ifdef CONFIG_SAFESTACK
>> +#error "SafeStack does not work with sigaltstack's implementation"
>> +#endif
> 
> Neither the commit description nor the #error message explain why it
> doesn't work. Could it work in the future or is there a fundamental
> reason why it will never work?
> 
> Stefan
> 

[PATCH 3/4] configure: add flag to enable SafeStack

[Daniele Buono]

This patch adds a flag to enable the SafeStack instrumentation provided
by LLVM.
The checks make sure that the compiler supports the flags, and that we
are using the proper coroutine implementation (coroutine-ucontext).
While SafeStack is supported only on Linux, NetBSD, FreeBSD and macOS,
we are not checking for the O.S. since this is already done by LLVM.

Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
---
 configure | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/configure b/configure
index 23b5e93752..f37e4ae0bd 100755
--- a/configure
+++ b/configure
@@ -302,6 +302,7 @@ audio_win_int=""
 libs_qga=""
 debug_info="yes"
 stack_protector=""
+safe_stack="no"
 use_containers="yes"
 gdb_bin=$(command -v "gdb")
 
@@ -1275,6 +1276,8 @@ for opt do
   ;;
   --disable-stack-protector) stack_protector="no"
   ;;
+  --enable-safe-stack) safe_stack="yes"
+  ;;
   --disable-curses) curses="no"
   ;;
   --enable-curses) curses="yes"
@@ -1774,6 +1777,8 @@ Advanced options (experts only):
   --with-coroutine=BACKEND coroutine backend. Supported options:
                            ucontext, sigaltstack, windows
   --enable-gcov            enable test coverage analysis with gcov
+  --enable-safe-stack      enable the SafeStack stack protection. Depends on
+                           clang/llvm >= 3.7 and coroutine backend ucontext.
   --gcov=GCOV              use specified gcov [$gcov_tool]
   --disable-blobs          disable installing provided firmware blobs
   --with-vss-sdk=SDK-path  enable Windows VSS support in QEMU Guest Agent
@@ -5501,6 +5506,29 @@ if test "$debug_stack_usage" = "yes"; then
   fi
 fi
 
+##################################################
+# Check if SafeStack is enabled and supported
+
+if test "$safe_stack" = "yes"; then
+  cat > $TMPC << EOF
+int main(int argc, char *argv[])
+{
+    return 0;
+}
+EOF
+  flag="-fsanitize=safe-stack"
+  # Check that safe-stack is supported.
+  if compile_prog "-Werror $flag" ""; then
+    # Flag needed both at compilation and at linking
+    QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+    QEMU_LDFLAGS="$QEMU_LDFLAGS $flag"
+  else
+    error_exit "SafeStack not supported by your compiler"
+  fi
+  if test "$coroutine" != "ucontext"; then
+    error_exit "SafeStack is only supported by the coroutine backend ucontext"
+  fi
+fi
 
 ##########################################
 # check if we have open_by_handle_at
@@ -6595,6 +6623,7 @@ echo "sparse enabled    $sparse"
 echo "strip binaries    $strip_opt"
 echo "profiler          $profiler"
 echo "static build      $static"
+echo "safe stack        $safe_stack"
 if test "$darwin" = "yes" ; then
     echo "Cocoa support     $cocoa"
 fi
-- 
2.26.2

Re: [PATCH 3/4] configure: add flag to enable SafeStack

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

On Wed, Apr 29, 2020 at 03:44:19PM -0400, Daniele Buono wrote:
> This patch adds a flag to enable the SafeStack instrumentation provided
> by LLVM.
> The checks make sure that the compiler supports the flags, and that we
> are using the proper coroutine implementation (coroutine-ucontext).
> While SafeStack is supported only on Linux, NetBSD, FreeBSD and macOS,
> we are not checking for the O.S. since this is already done by LLVM.
> 
> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> ---
>  configure | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

Great, this can become Patch 1 and it can set CONFIG_SAFESTACK as
mentioned in my earlier reply.

> diff --git a/configure b/configure
> index 23b5e93752..f37e4ae0bd 100755
> --- a/configure
> +++ b/configure
> @@ -302,6 +302,7 @@ audio_win_int=""
>  libs_qga=""
>  debug_info="yes"
>  stack_protector=""
> +safe_stack="no"

The comment above this says:

  # Always add --enable-foo and --disable-foo command line args.

Please add --disable-safe-stack.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] configure: add flag to enable SafeStack

[Daniele Buono]

Hi Stefan,

I would feel more confident by adding another check in configure to make
sure that the user didn't enable SafeStack manually through other means,
like manually setting the option through extra_cflags.
What do you think?

On 5/21/2020 5:52 AM, Stefan Hajnoczi wrote:
> On Wed, Apr 29, 2020 at 03:44:19PM -0400, Daniele Buono wrote:
>> This patch adds a flag to enable the SafeStack instrumentation provided
>> by LLVM.
>> The checks make sure that the compiler supports the flags, and that we
>> are using the proper coroutine implementation (coroutine-ucontext).
>> While SafeStack is supported only on Linux, NetBSD, FreeBSD and macOS,
>> we are not checking for the O.S. since this is already done by LLVM.
>>
>> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
>> ---
>>   configure | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
> 
> Great, this can become Patch 1 and it can set CONFIG_SAFESTACK as
> mentioned in my earlier reply.
> 
>> diff --git a/configure b/configure
>> index 23b5e93752..f37e4ae0bd 100755
>> --- a/configure
>> +++ b/configure
>> @@ -302,6 +302,7 @@ audio_win_int=""
>>   libs_qga=""
>>   debug_info="yes"
>>   stack_protector=""
>> +safe_stack="no"
> 
> The comment above this says:
> 
>    # Always add --enable-foo and --disable-foo command line args.
> 
> Please add --disable-safe-stack.
> 

Re: [PATCH 3/4] configure: add flag to enable SafeStack

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

On Fri, May 22, 2020 at 11:24:46AM -0400, Daniele Buono wrote:
> I would feel more confident by adding another check in configure to make
> sure that the user didn't enable SafeStack manually through other means,
> like manually setting the option through extra_cflags.
> What do you think?

Sure, a compile_prog call could check if SafeStack is enable when it
shouldn't be.

This can be done together with a --disable option.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] configure: add flag to enable SafeStack

[Daniele Buono]

Hi Stefan, a quick clarification on configure:

right now, in configure, there's
* "Advanced Options (experts only)"
which usually don't have both enable and disable for each option, and
* "Optional features, enabled with --enable-FEATURE and
disabled with --disable-FEATURE, default is enabled if available:"

How do you think SafeStack should be classified?
* If we do it as Advanced option, we should probably force it disabled
unless --enable-safe-stack is provided. In this case
--disable-safe-stack is not really necessary.
* If we do it as optional feature, I have two ways to handle the default:
1. enable/disable based on the compilation flags given to configure
2. enable every time the provided compiler supports it

On 5/27/2020 7:12 AM, Stefan Hajnoczi wrote:
> On Fri, May 22, 2020 at 11:24:46AM -0400, Daniele Buono wrote:
>> I would feel more confident by adding another check in configure to make
>> sure that the user didn't enable SafeStack manually through other means,
>> like manually setting the option through extra_cflags.
>> What do you think?
> 
> Sure, a compile_prog call could check if SafeStack is enable when it
> shouldn't be.
> 
> This can be done together with a --disable option.
> 
> Stefan
> 

[PATCH 4/4] check-block: Enable iotests with SafeStack

[Daniele Buono]

SafeStack is a stack protection technique implemented in llvm. It is
enabled with a -fsanitize flag.
iotests are currently disabled when any -fsanitize option is used.
Since SafeStack is useful on production environments, and its
implementation may break the binary, filter it out when the check is
performed, so that if SafeStack was the only -fsanitize option, iotests
are still performed.

Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
---
 tests/check-block.sh | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index ad320c21ba..8e29c868e5 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -21,7 +21,17 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; then
     exit 0
 fi
 
-if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then
+# Disable tests with any sanitizer except for SafeStack
+CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
+SANITIZE_FLAGS=""
+#Remove all occurrencies of -fsanitize=safe-stack
+for i in ${CFLAGS}; do
+        if [ "${i}" != "-fsanitize=safe-stack" ]; then
+                SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
+        fi
+done
+if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
+    # Have a sanitize flag that is not allowed, stop
     echo "Sanitizers are enabled ==> Not running the qemu-iotests."
     exit 0
 fi
-- 
2.26.2

Re: [PATCH 4/4] check-block: Enable iotests with SafeStack

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

On Wed, Apr 29, 2020 at 03:44:20PM -0400, Daniele Buono wrote:
> SafeStack is a stack protection technique implemented in llvm. It is
> enabled with a -fsanitize flag.
> iotests are currently disabled when any -fsanitize option is used.
> Since SafeStack is useful on production environments, and its
> implementation may break the binary, filter it out when the check is
> performed, so that if SafeStack was the only -fsanitize option, iotests
> are still performed.

I can't parse this sentence. What does "its implementation may break the
binary" mean? Do you mean it's worth running tests with SafeStack
enabled because it exposes failures that go unnoticed without SafeStack?

> 
> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> ---
>  tests/check-block.sh | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/check-block.sh b/tests/check-block.sh
> index ad320c21ba..8e29c868e5 100755
> --- a/tests/check-block.sh
> +++ b/tests/check-block.sh
> @@ -21,7 +21,17 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; then
>      exit 0
>  fi
>  
> -if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then
> +# Disable tests with any sanitizer except for SafeStack
> +CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
> +SANITIZE_FLAGS=""
> +#Remove all occurrencies of -fsanitize=safe-stack
> +for i in ${CFLAGS}; do
> +        if [ "${i}" != "-fsanitize=safe-stack" ]; then
> +                SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
> +        fi
> +done
> +if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
> +    # Have a sanitize flag that is not allowed, stop
>      echo "Sanitizers are enabled ==> Not running the qemu-iotests."
>      exit 0
>  fi

The commit that disabled check-block.sh with sanitizers said:

  The sanitizers (especially the address sanitizer from Clang) are
  sometimes printing out warnings or false positives - this spoils
  the output of the iotests, causing some of the tests to fail.

It seems fine to allow SafeStack if check-block.sh currently passes with
it enabled. Does it pass and produce no extra output?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] check-block: Enable iotests with SafeStack

[Daniele Buono]

On 5/21/2020 5:59 AM, Stefan Hajnoczi wrote:
> On Wed, Apr 29, 2020 at 03:44:20PM -0400, Daniele Buono wrote:
>> SafeStack is a stack protection technique implemented in llvm. It is
>> enabled with a -fsanitize flag.
>> iotests are currently disabled when any -fsanitize option is used.
>> Since SafeStack is useful on production environments, and its
>> implementation may break the binary, filter it out when the check is
>> performed, so that if SafeStack was the only -fsanitize option, iotests
>> are still performed.
> 
> I can't parse this sentence. What does "its implementation may break the
> binary" mean? Do you mean it's worth running tests with SafeStack
> enabled because it exposes failures that go unnoticed without SafeStack?

What I meant is that, without proper changes, SafeStack breaks 
co-routines. Since they are heavily used in the io subsystem, this is 
probably the best class of tests to make sure co-routines are working 
fine with SafeStack.

I initially re-enabled the iotests for my internal testing.

Since I wasn't seeing any issue, I thought it would be useful to keep 
running this to make sure future implementations of SafeStack won't 
break co-routines again.

> 
>>
>> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
>> ---
>>   tests/check-block.sh | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/check-block.sh b/tests/check-block.sh
>> index ad320c21ba..8e29c868e5 100755
>> --- a/tests/check-block.sh
>> +++ b/tests/check-block.sh
>> @@ -21,7 +21,17 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; then
>>       exit 0
>>   fi
>>   
>> -if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then
>> +# Disable tests with any sanitizer except for SafeStack
>> +CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
>> +SANITIZE_FLAGS=""
>> +#Remove all occurrencies of -fsanitize=safe-stack
>> +for i in ${CFLAGS}; do
>> +        if [ "${i}" != "-fsanitize=safe-stack" ]; then
>> +                SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
>> +        fi
>> +done
>> +if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
>> +    # Have a sanitize flag that is not allowed, stop
>>       echo "Sanitizers are enabled ==> Not running the qemu-iotests."
>>       exit 0
>>   fi
> 
> The commit that disabled check-block.sh with sanitizers said:
> 
>    The sanitizers (especially the address sanitizer from Clang) are
>    sometimes printing out warnings or false positives - this spoils
>    the output of the iotests, causing some of the tests to fail.
> 
> It seems fine to allow SafeStack if check-block.sh currently passes with
> it enabled. Does it pass and produce no extra output?
> 
Yes, that was the idea. SafeStack should pass the tests without extra 
output.

It did (pass) on my testing machine. However I don't remember if I did 
the full (slow) check or only the partial one.

Will check again before I submit v2
> Stefan
> 

Re: [PATCH 4/4] check-block: Enable iotests with SafeStack

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

On Fri, May 22, 2020 at 11:35:42AM -0400, Daniele Buono wrote:
> On 5/21/2020 5:59 AM, Stefan Hajnoczi wrote:
> > On Wed, Apr 29, 2020 at 03:44:20PM -0400, Daniele Buono wrote:
> > 
> > > 
> > > Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> > > ---
> > >   tests/check-block.sh | 12 +++++++++++-
> > >   1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/check-block.sh b/tests/check-block.sh
> > > index ad320c21ba..8e29c868e5 100755
> > > --- a/tests/check-block.sh
> > > +++ b/tests/check-block.sh
> > > @@ -21,7 +21,17 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; then
> > >       exit 0
> > >   fi
> > > -if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then
> > > +# Disable tests with any sanitizer except for SafeStack
> > > +CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
> > > +SANITIZE_FLAGS=""
> > > +#Remove all occurrencies of -fsanitize=safe-stack
> > > +for i in ${CFLAGS}; do
> > > +        if [ "${i}" != "-fsanitize=safe-stack" ]; then
> > > +                SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
> > > +        fi
> > > +done
> > > +if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
> > > +    # Have a sanitize flag that is not allowed, stop
> > >       echo "Sanitizers are enabled ==> Not running the qemu-iotests."
> > >       exit 0
> > >   fi
> > 
> > The commit that disabled check-block.sh with sanitizers said:
> > 
> >    The sanitizers (especially the address sanitizer from Clang) are
> >    sometimes printing out warnings or false positives - this spoils
> >    the output of the iotests, causing some of the tests to fail.
> > 
> > It seems fine to allow SafeStack if check-block.sh currently passes with
> > it enabled. Does it pass and produce no extra output?
> > 
> Yes, that was the idea. SafeStack should pass the tests without extra
> output.
> 
> It did (pass) on my testing machine. However I don't remember if I did the
> full (slow) check or only the partial one.
> 
> Will check again before I submit v2

Great, thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/4] Add support for SafeStack

[Philippe Mathieu-Daudé]

On 4/29/20 9:44 PM, Daniele Buono wrote:
> LLVM supports SafeStack instrumentation to protect against stack buffer
> overflows, since version 3.7
> 
>  From https://clang.llvm.org/docs/SafeStack.html:
> "It works by separating the program stack into two distinct regions: the
> safe stack and the unsafe stack. The safe stack stores return addresses,
> register spills, and local variables that are always accessed in a safe
> way, while the unsafe stack stores everything else. This separation
> ensures that buffer overflows on the unsafe stack cannot be used to
> overwrite anything on the safe stack."
> 
> Unfortunately, the use of two stack regions does not cope well with
> QEMU's coroutines. The second stack region is not properly set up with
> both ucontext and sigaltstack, so multiple coroutines end up sharing the
> same memory area for the unsafe stack, causing undefined behaviors at
> runtime (and most iochecks to fail).
> 
> This patch series fixes the implementation of the ucontext backend and
> make sure that sigaltstack is never used if the compiler is applying
> the SafeStack instrumentation. It also adds a configure flag to enable
> SafeStack, and enables iotests when SafeStack is used.
> 
> This is an RFC mainly because of the low-level use of the SafeStack
> runtime.
> When running swapcontext(), we have to manually set the unsafe stack
> pointer to the new area allocated for the coroutine. LLVM does not allow
> this by using builtin, so we have to use implementation details that may
> change in the future.
> This patch has been tested briefly ( make check on an x86 system ) with
> clang v3.9, v4.0, v5.0, v6.0
> Heavier testing, with make check-acceptance has been performed with
> clang v7.0

Clang10:
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Daniele Buono (4):
>    coroutine: support SafeStack in ucontext backend
>    coroutine: Add check for SafeStack in sigalstack
>    configure: add flag to enable SafeStack
>    check-block: Enable iotests with SafeStack
> 
>   configure                    | 29 +++++++++++++++++++++++++++++
>   include/qemu/coroutine_int.h |  6 ++++++
>   tests/check-block.sh         | 12 +++++++++++-
>   util/coroutine-sigaltstack.c |  4 ++++
>   util/coroutine-ucontext.c    | 25 +++++++++++++++++++++++++
>   5 files changed, 75 insertions(+), 1 deletion(-)
> 

Re: [PATCH 0/4] Add support for SafeStack

[Philippe Mathieu-Daudé]

+Alex & Daniel who keep track on CI stuff.

On 4/29/20 9:44 PM, Daniele Buono wrote:
> LLVM supports SafeStack instrumentation to protect against stack buffer
> overflows, since version 3.7
> 
>  From https://clang.llvm.org/docs/SafeStack.html:
> "It works by separating the program stack into two distinct regions: the
> safe stack and the unsafe stack. The safe stack stores return addresses,
> register spills, and local variables that are always accessed in a safe
> way, while the unsafe stack stores everything else. This separation
> ensures that buffer overflows on the unsafe stack cannot be used to
> overwrite anything on the safe stack."
> 
> Unfortunately, the use of two stack regions does not cope well with
> QEMU's coroutines. The second stack region is not properly set up with
> both ucontext and sigaltstack, so multiple coroutines end up sharing the
> same memory area for the unsafe stack, causing undefined behaviors at
> runtime (and most iochecks to fail).
> 
> This patch series fixes the implementation of the ucontext backend and
> make sure that sigaltstack is never used if the compiler is applying
> the SafeStack instrumentation. It also adds a configure flag to enable
> SafeStack, and enables iotests when SafeStack is used.
> 
> This is an RFC mainly because of the low-level use of the SafeStack
> runtime.
> When running swapcontext(), we have to manually set the unsafe stack
> pointer to the new area allocated for the coroutine. LLVM does not allow
> this by using builtin, so we have to use implementation details that may
> change in the future.
> This patch has been tested briefly ( make check on an x86 system ) with
> clang v3.9, v4.0, v5.0, v6.0
> Heavier testing, with make check-acceptance has been performed with
> clang v7.0

I noticed building using SafeStack is slower, and running with it is 
even sloooower. It makes sense to have this integrated if we use it 
regularly. Do you have plan for this? Using public CI doesn't seem 
reasonable.

> 
> Daniele Buono (4):
>    coroutine: support SafeStack in ucontext backend
>    coroutine: Add check for SafeStack in sigalstack
>    configure: add flag to enable SafeStack
>    check-block: Enable iotests with SafeStack
> 
>   configure                    | 29 +++++++++++++++++++++++++++++
>   include/qemu/coroutine_int.h |  6 ++++++
>   tests/check-block.sh         | 12 +++++++++++-
>   util/coroutine-sigaltstack.c |  4 ++++
>   util/coroutine-ucontext.c    | 25 +++++++++++++++++++++++++
>   5 files changed, 75 insertions(+), 1 deletion(-)
> 

Re: [PATCH 0/4] Add support for SafeStack

[Daniel P. Berrangé]

On Tue, May 05, 2020 at 03:15:18PM +0200, Philippe Mathieu-Daudé wrote:
> +Alex & Daniel who keep track on CI stuff.
> 
> On 4/29/20 9:44 PM, Daniele Buono wrote:
> > LLVM supports SafeStack instrumentation to protect against stack buffer
> > overflows, since version 3.7
> > 
> >  From https://clang.llvm.org/docs/SafeStack.html:
> > "It works by separating the program stack into two distinct regions: the
> > safe stack and the unsafe stack. The safe stack stores return addresses,
> > register spills, and local variables that are always accessed in a safe
> > way, while the unsafe stack stores everything else. This separation
> > ensures that buffer overflows on the unsafe stack cannot be used to
> > overwrite anything on the safe stack."
> > 
> > Unfortunately, the use of two stack regions does not cope well with
> > QEMU's coroutines. The second stack region is not properly set up with
> > both ucontext and sigaltstack, so multiple coroutines end up sharing the
> > same memory area for the unsafe stack, causing undefined behaviors at
> > runtime (and most iochecks to fail).
> > 
> > This patch series fixes the implementation of the ucontext backend and
> > make sure that sigaltstack is never used if the compiler is applying
> > the SafeStack instrumentation. It also adds a configure flag to enable
> > SafeStack, and enables iotests when SafeStack is used.
> > 
> > This is an RFC mainly because of the low-level use of the SafeStack
> > runtime.
> > When running swapcontext(), we have to manually set the unsafe stack
> > pointer to the new area allocated for the coroutine. LLVM does not allow
> > this by using builtin, so we have to use implementation details that may
> > change in the future.
> > This patch has been tested briefly ( make check on an x86 system ) with
> > clang v3.9, v4.0, v5.0, v6.0
> > Heavier testing, with make check-acceptance has been performed with
> > clang v7.0
> 
> I noticed building using SafeStack is slower, and running with it is even
> sloooower. It makes sense to have this integrated if we use it regularly. Do
> you have plan for this? Using public CI doesn't seem reasonable.

The runtime behaviour is rather odd, given the docs they provide:

"The performance overhead of the SafeStack instrumentation is
 less than 0.1% on average across a variety of benchmarks 
 This is mainly because most small functions do not have any
 variables that require the unsafe stack and, hence, do not 
 need unsafe stack frames to be created. The cost of creating 
 unsafe stack frames for large functions is amortized by the 
 cost of executing the function.

  In some cases, SafeStack actually improves the performance"

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/4] Add support for SafeStack

[Philippe Mathieu-Daudé]

On 5/5/20 3:31 PM, Daniel P. Berrangé wrote:
> On Tue, May 05, 2020 at 03:15:18PM +0200, Philippe Mathieu-Daudé wrote:
>> +Alex & Daniel who keep track on CI stuff.
>>
>> On 4/29/20 9:44 PM, Daniele Buono wrote:
>>> LLVM supports SafeStack instrumentation to protect against stack buffer
>>> overflows, since version 3.7
>>>
>>>   From https://clang.llvm.org/docs/SafeStack.html:
>>> "It works by separating the program stack into two distinct regions: the
>>> safe stack and the unsafe stack. The safe stack stores return addresses,
>>> register spills, and local variables that are always accessed in a safe
>>> way, while the unsafe stack stores everything else. This separation
>>> ensures that buffer overflows on the unsafe stack cannot be used to
>>> overwrite anything on the safe stack."
>>>
>>> Unfortunately, the use of two stack regions does not cope well with
>>> QEMU's coroutines. The second stack region is not properly set up with
>>> both ucontext and sigaltstack, so multiple coroutines end up sharing the
>>> same memory area for the unsafe stack, causing undefined behaviors at
>>> runtime (and most iochecks to fail).
>>>
>>> This patch series fixes the implementation of the ucontext backend and
>>> make sure that sigaltstack is never used if the compiler is applying
>>> the SafeStack instrumentation. It also adds a configure flag to enable
>>> SafeStack, and enables iotests when SafeStack is used.
>>>
>>> This is an RFC mainly because of the low-level use of the SafeStack
>>> runtime.
>>> When running swapcontext(), we have to manually set the unsafe stack
>>> pointer to the new area allocated for the coroutine. LLVM does not allow
>>> this by using builtin, so we have to use implementation details that may
>>> change in the future.
>>> This patch has been tested briefly ( make check on an x86 system ) with
>>> clang v3.9, v4.0, v5.0, v6.0
>>> Heavier testing, with make check-acceptance has been performed with
>>> clang v7.0
>>
>> I noticed building using SafeStack is slower, and running with it is even
>> sloooower. It makes sense to have this integrated if we use it regularly. Do
>> you have plan for this? Using public CI doesn't seem reasonable.
> 
> The runtime behaviour is rather odd, given the docs they provide:
> 
> "The performance overhead of the SafeStack instrumentation is
>   less than 0.1% on average across a variety of benchmarks
>   This is mainly because most small functions do not have any
>   variables that require the unsafe stack and, hence, do not
>   need unsafe stack frames to be created. The cost of creating
>   unsafe stack frames for large functions is amortized by the
>   cost of executing the function.
> 
>    In some cases, SafeStack actually improves the performance"

I'm sorry I was testing this with a single core instead of all of 
them... Thanks for looking at the doc.

> 
> Regards,
> Daniel
> 

Re: [PATCH 0/4] Add support for SafeStack

[Daniele Buono]

Hello everybody, just pinging since it it's been a few days.

On 5/5/2020 9:56 AM, Philippe Mathieu-Daudé wrote:
> On 5/5/20 3:31 PM, Daniel P. Berrangé wrote:
>> On Tue, May 05, 2020 at 03:15:18PM +0200, Philippe Mathieu-Daudé wrote:
>>> +Alex & Daniel who keep track on CI stuff.
>>>
>>> On 4/29/20 9:44 PM, Daniele Buono wrote:
>>>> LLVM supports SafeStack instrumentation to protect against stack buffer
>>>> overflows, since version 3.7
>>>>
>>>>   From https://clang.llvm.org/docs/SafeStack.html:
>>>> "It works by separating the program stack into two distinct regions: 
>>>> the
>>>> safe stack and the unsafe stack. The safe stack stores return 
>>>> addresses,
>>>> register spills, and local variables that are always accessed in a safe
>>>> way, while the unsafe stack stores everything else. This separation
>>>> ensures that buffer overflows on the unsafe stack cannot be used to
>>>> overwrite anything on the safe stack."
>>>>
>>>> Unfortunately, the use of two stack regions does not cope well with
>>>> QEMU's coroutines. The second stack region is not properly set up with
>>>> both ucontext and sigaltstack, so multiple coroutines end up sharing 
>>>> the
>>>> same memory area for the unsafe stack, causing undefined behaviors at
>>>> runtime (and most iochecks to fail).
>>>>
>>>> This patch series fixes the implementation of the ucontext backend and
>>>> make sure that sigaltstack is never used if the compiler is applying
>>>> the SafeStack instrumentation. It also adds a configure flag to enable
>>>> SafeStack, and enables iotests when SafeStack is used.
>>>>
>>>> This is an RFC mainly because of the low-level use of the SafeStack
>>>> runtime.
>>>> When running swapcontext(), we have to manually set the unsafe stack
>>>> pointer to the new area allocated for the coroutine. LLVM does not 
>>>> allow
>>>> this by using builtin, so we have to use implementation details that 
>>>> may
>>>> change in the future.
>>>> This patch has been tested briefly ( make check on an x86 system ) with
>>>> clang v3.9, v4.0, v5.0, v6.0
>>>> Heavier testing, with make check-acceptance has been performed with
>>>> clang v7.0
>>>
>>> I noticed building using SafeStack is slower, and running with it is 
>>> even
>>> sloooower. It makes sense to have this integrated if we use it 
>>> regularly. Do
>>> you have plan for this? Using public CI doesn't seem reasonable.
>>
>> The runtime behaviour is rather odd, given the docs they provide:
>>
>> "The performance overhead of the SafeStack instrumentation is
>>   less than 0.1% on average across a variety of benchmarks
>>   This is mainly because most small functions do not have any
>>   variables that require the unsafe stack and, hence, do not
>>   need unsafe stack frames to be created. The cost of creating
>>   unsafe stack frames for large functions is amortized by the
>>   cost of executing the function.
>>
>>    In some cases, SafeStack actually improves the performance"
> 
> I'm sorry I was testing this with a single core instead of all of 
> them... Thanks for looking at the doc.
> 
>>
>> Regards,
>> Daniel
>>
> 
> 

Re: [PATCH 0/4] Add support for SafeStack

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

On Wed, May 13, 2020 at 10:48:04AM -0400, Daniele Buono wrote:
> Hello everybody, just pinging since it it's been a few days.

Hi Daniele,
Sorry I'm late to the party. This looks useful, the patches are not
invasive and provide the option of enabling extra security.

I have left comments on specific patches.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]