* [PATCH 1/4] coroutine: support SafeStack in ucontext backend
2020-04-29 19:44 [PATCH 0/4] Add support for SafeStack Daniele Buono
@ 2020-04-29 19:44 ` Daniele Buono
2020-05-21 9:44 ` Stefan Hajnoczi
2020-04-29 19:44 ` [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack Daniele Buono
` (4 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Daniele Buono @ 2020-04-29 19:44 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, Daniele Buono,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend
2020-04-29 19:44 ` [PATCH 1/4] coroutine: support SafeStack in ucontext backend Daniele Buono
@ 2020-05-21 9:44 ` Stefan Hajnoczi
2020-05-22 15:18 ` Daniele Buono
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2020-05-21 9:44 UTC (permalink / raw)
To: Daniele Buono
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, qemu-devel,
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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend
2020-05-21 9:44 ` Stefan Hajnoczi
@ 2020-05-22 15:18 ` Daniele Buono
2020-05-27 10:34 ` Stefan Hajnoczi
0 siblings, 1 reply; 24+ messages in thread
From: Daniele Buono @ 2020-05-22 15:18 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, qemu-devel,
Stefan Hajnoczi
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);
>> }
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend
2020-05-22 15:18 ` Daniele Buono
@ 2020-05-27 10:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2020-05-27 10:34 UTC (permalink / raw)
To: Daniele Buono
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, qemu-devel,
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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack
2020-04-29 19:44 [PATCH 0/4] Add support for SafeStack Daniele Buono
2020-04-29 19:44 ` [PATCH 1/4] coroutine: support SafeStack in ucontext backend Daniele Buono
@ 2020-04-29 19:44 ` Daniele Buono
2020-05-04 14:56 ` Philippe Mathieu-Daudé
2020-05-21 9:49 ` Stefan Hajnoczi
2020-04-29 19:44 ` [PATCH 3/4] configure: add flag to enable SafeStack Daniele Buono
` (3 subsequent siblings)
5 siblings, 2 replies; 24+ messages in thread
From: Daniele Buono @ 2020-04-29 19:44 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, Daniele Buono,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack
2020-04-29 19:44 ` [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack Daniele Buono
@ 2020-05-04 14:56 ` Philippe Mathieu-Daudé
2020-05-21 9:49 ` Stefan Hajnoczi
1 sibling, 0 replies; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-04 14:56 UTC (permalink / raw)
To: Daniele Buono, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, Stefan Hajnoczi
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack
2020-04-29 19:44 ` [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack Daniele Buono
2020-05-04 14:56 ` Philippe Mathieu-Daudé
@ 2020-05-21 9:49 ` Stefan Hajnoczi
2020-05-27 17:56 ` Daniele Buono
1 sibling, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2020-05-21 9:49 UTC (permalink / raw)
To: Daniele Buono
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, qemu-devel,
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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack
2020-05-21 9:49 ` Stefan Hajnoczi
@ 2020-05-27 17:56 ` Daniele Buono
0 siblings, 0 replies; 24+ messages in thread
From: Daniele Buono @ 2020-05-27 17:56 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, qemu-devel,
Stefan Hajnoczi
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/4] configure: add flag to enable SafeStack
2020-04-29 19:44 [PATCH 0/4] Add support for SafeStack Daniele Buono
2020-04-29 19:44 ` [PATCH 1/4] coroutine: support SafeStack in ucontext backend Daniele Buono
2020-04-29 19:44 ` [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack Daniele Buono
@ 2020-04-29 19:44 ` Daniele Buono
2020-05-21 9:52 ` Stefan Hajnoczi
2020-04-29 19:44 ` [PATCH 4/4] check-block: Enable iotests with SafeStack Daniele Buono
` (2 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Daniele Buono @ 2020-04-29 19:44 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, Daniele Buono,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 3/4] configure: add flag to enable SafeStack
2020-04-29 19:44 ` [PATCH 3/4] configure: add flag to enable SafeStack Daniele Buono
@ 2020-05-21 9:52 ` Stefan Hajnoczi
2020-05-22 15:24 ` Daniele Buono
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2020-05-21 9:52 UTC (permalink / raw)
To: Daniele Buono
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, qemu-devel,
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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/4] configure: add flag to enable SafeStack
2020-05-21 9:52 ` Stefan Hajnoczi
@ 2020-05-22 15:24 ` Daniele Buono
2020-05-27 11:12 ` Stefan Hajnoczi
0 siblings, 1 reply; 24+ messages in thread
From: Daniele Buono @ 2020-05-22 15:24 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, qemu-devel,
Stefan Hajnoczi
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.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/4] configure: add flag to enable SafeStack
2020-05-22 15:24 ` Daniele Buono
@ 2020-05-27 11:12 ` Stefan Hajnoczi
2020-05-27 13:48 ` Daniele Buono
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2020-05-27 11:12 UTC (permalink / raw)
To: Daniele Buono
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, qemu-devel,
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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/4] configure: add flag to enable SafeStack
2020-05-27 11:12 ` Stefan Hajnoczi
@ 2020-05-27 13:48 ` Daniele Buono
0 siblings, 0 replies; 24+ messages in thread
From: Daniele Buono @ 2020-05-27 13:48 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, qemu-devel,
Stefan Hajnoczi
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] check-block: Enable iotests with SafeStack
2020-04-29 19:44 [PATCH 0/4] Add support for SafeStack Daniele Buono
` (2 preceding siblings ...)
2020-04-29 19:44 ` [PATCH 3/4] configure: add flag to enable SafeStack Daniele Buono
@ 2020-04-29 19:44 ` Daniele Buono
2020-05-21 9:59 ` Stefan Hajnoczi
2020-05-04 14:55 ` [PATCH 0/4] Add support for SafeStack Philippe Mathieu-Daudé
2020-05-05 13:15 ` Philippe Mathieu-Daudé
5 siblings, 1 reply; 24+ messages in thread
From: Daniele Buono @ 2020-04-29 19:44 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, Daniele Buono,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] check-block: Enable iotests with SafeStack
2020-04-29 19:44 ` [PATCH 4/4] check-block: Enable iotests with SafeStack Daniele Buono
@ 2020-05-21 9:59 ` Stefan Hajnoczi
2020-05-22 15:35 ` Daniele Buono
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2020-05-21 9:59 UTC (permalink / raw)
To: Daniele Buono
Cc: Kevin Wolf, thuth, Tobin Feldman-Fitzthum, qemu-devel,
Stefan Hajnoczi, Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] check-block: Enable iotests with SafeStack
2020-05-21 9:59 ` Stefan Hajnoczi
@ 2020-05-22 15:35 ` Daniele Buono
2020-05-27 11:13 ` Stefan Hajnoczi
0 siblings, 1 reply; 24+ messages in thread
From: Daniele Buono @ 2020-05-22 15:35 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, thuth, Tobin Feldman-Fitzthum, qemu-devel,
Stefan Hajnoczi, Paolo Bonzini
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] check-block: Enable iotests with SafeStack
2020-05-22 15:35 ` Daniele Buono
@ 2020-05-27 11:13 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2020-05-27 11:13 UTC (permalink / raw)
To: Daniele Buono
Cc: Kevin Wolf, thuth, Tobin Feldman-Fitzthum, qemu-devel,
Stefan Hajnoczi, Paolo Bonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] Add support for SafeStack
2020-04-29 19:44 [PATCH 0/4] Add support for SafeStack Daniele Buono
` (3 preceding siblings ...)
2020-04-29 19:44 ` [PATCH 4/4] check-block: Enable iotests with SafeStack Daniele Buono
@ 2020-05-04 14:55 ` Philippe Mathieu-Daudé
2020-05-05 13:15 ` Philippe Mathieu-Daudé
5 siblings, 0 replies; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-04 14:55 UTC (permalink / raw)
To: Daniele Buono, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Tobin Feldman-Fitzthum, Stefan Hajnoczi
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(-)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] Add support for SafeStack
2020-04-29 19:44 [PATCH 0/4] Add support for SafeStack Daniele Buono
` (4 preceding siblings ...)
2020-05-04 14:55 ` [PATCH 0/4] Add support for SafeStack Philippe Mathieu-Daudé
@ 2020-05-05 13:15 ` Philippe Mathieu-Daudé
2020-05-05 13:31 ` Daniel P. Berrangé
5 siblings, 1 reply; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-05 13:15 UTC (permalink / raw)
To: Daniele Buono, qemu-devel
Cc: Kevin Wolf, Daniel P . Berrange, Tobin Feldman-Fitzthum,
Stefan Hajnoczi, Paolo Bonzini, Alex Bennée
+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(-)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] Add support for SafeStack
2020-05-05 13:15 ` Philippe Mathieu-Daudé
@ 2020-05-05 13:31 ` Daniel P. Berrangé
2020-05-05 13:56 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 24+ messages in thread
From: Daniel P. Berrangé @ 2020-05-05 13:31 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Tobin Feldman-Fitzthum, qemu-devel, Stefan Hajnoczi,
Paolo Bonzini, Alex Bennée, Daniele Buono
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 :|
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] Add support for SafeStack
2020-05-05 13:31 ` Daniel P. Berrangé
@ 2020-05-05 13:56 ` Philippe Mathieu-Daudé
2020-05-13 14:48 ` Daniele Buono
0 siblings, 1 reply; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-05 13:56 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Tobin Feldman-Fitzthum, qemu-devel, Stefan Hajnoczi,
Paolo Bonzini, Alex Bennée, Daniele Buono
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] Add support for SafeStack
2020-05-05 13:56 ` Philippe Mathieu-Daudé
@ 2020-05-13 14:48 ` Daniele Buono
2020-05-21 10:00 ` Stefan Hajnoczi
0 siblings, 1 reply; 24+ messages in thread
From: Daniele Buono @ 2020-05-13 14:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Daniel P. Berrangé
Cc: Kevin Wolf, Tobin Feldman-Fitzthum, qemu-devel, Stefan Hajnoczi,
Paolo Bonzini, Alex Bennée
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
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] Add support for SafeStack
2020-05-13 14:48 ` Daniele Buono
@ 2020-05-21 10:00 ` Stefan Hajnoczi
0 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2020-05-21 10:00 UTC (permalink / raw)
To: Daniele Buono
Cc: Kevin Wolf, Daniel P. Berrangé,
Alex Bennée, Tobin Feldman-Fitzthum, qemu-devel,
Stefan Hajnoczi, Paolo Bonzini, Philippe Mathieu-Daudé
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread