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

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>

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

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
>