Re: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with makecontext family functions

[H.J. Lu]

On Tue, Dec 19, 2017 at 4:41 AM, Andreas Schwab  wrote:
> On Dez 19 2017, "H.J. Lu"  wrote:
>
>> This isn't sufficient.  I did get
>>
>> ../sysdeps/unix/sysv/linux/x86/tst-ucontext-1a.c:25:10: error: call to
>> \u2018getcontext\u2019 declared with attribute error: -mshstk
>> incompatible with ucontext.h APIs
>>return getcontext (ucp);
>
> Any test for the ucontext functions will have to be compiled without
> -mshstk, of course.
>

Correct.

https://sourceware.org/ml/libc-alpha/2017-12/msg00638.html

has

(CFLAGS-tst-context1.c): Add -fcf-protection=branch -mno-shstk
-mibt.
(CFLAGS-bug-getcontext.c): Likewise.
(CFLAGS-tst-makecontext.c): Likewise.
(CFLAGS-tst-makecontext2.c): Likewise.
(CFLAGS-tst-makecontext3.c): Likewise.
(CFLAGS-tst-setcontext.c): Likewise.
(CFLAGS-tst-setcontext2.c): Likewise.
(CFLAGS-tst-setcontext3.c): Likewise.
(CFLAGS-tst-xbzero-opt.c): Likewise.

-- 
H.J.

Re: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with makecontext family functions

[Andreas Schwab]

On Dez 19 2017, "H.J. Lu"  wrote:

> This isn't sufficient.  I did get
>
> ../sysdeps/unix/sysv/linux/x86/tst-ucontext-1a.c:25:10: error: call to
> \u2018getcontext\u2019 declared with attribute error: -mshstk
> incompatible with ucontext.h APIs
>return getcontext (ucp);

Any test for the ucontext functions will have to be compiled without
-mshstk, of course.

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with makecontext family functions

[H.J. Lu]

On Tue, Dec 19, 2017 at 2:25 AM, Jakub Jelinek  wrote:
> On Tue, Dec 19, 2017 at 09:42:11AM +, Tsimbalist, Igor V wrote:
>> > I don't see any discussion in the bugzilla issue to explain this.
>>
>> This option is needed to support  two cases:
>>
>> 1. Compilation of ucontext functions inside glibc. To have glibc itself be 
>> CET
>> compatible all files comprises the library has to be CET compatible. That 
>> means
>> the module with ucontext functions from glibc has to be forced to be CET
>> compatible.
>>
>> 2. Compilation of a user application with ucontext functions. In this case 
>> the
>> error has to be issued, so no usage of a ucontext functions.
>>
>> Having just __SHSTK__ macro it's impossible to handle both cases. The case
>> #1 will report an error during glibc compilation. A new macro is introduced 
>> to
>> use in the source to handle these cases. To control the value of the new 
>> macro
>> a new option is introduced.
>
> That doesn't make sense.  Just add
>
> #if defined(__SHSTK__) && !defined(_LIBC)
> #define __UCONTEXT_UNSUPPORTED \
>   __attribute__((__error__ ("-mshstk incompatible with ucontext.h APIs")))
> #else
> #define __UCONTEXT_UNSUPPORTED
> #endif
>
> and use __UCONTEXT_UNSUPPORTED on the 4 function declarations, you'll be
> able to compile glibc itself, but not something against it.

This isn't sufficient.  I did get

../sysdeps/unix/sysv/linux/x86/tst-ucontext-1a.c:25:10: error: call to
\u2018getcontext\u2019 declared with attribute error: -mshstk
incompatible with ucontext.h APIs
   return getcontext (ucp);

But there is no error for

void *p = getcontext;

I am using

#if defined(__SHSTK__) && !defined(_LIBC)
# error -mshstk incompatible with ucontext.h APIs
#endif

> Have you considered implementing these 4 functions or some helper they could
> use in the kernel if CET is enabled and doing there with the shadow stack
> whatever is needed?
>

The main issue is how to allocate and destroy shadow stack within these
functions.  We will revisit it after the rest of glibc is CET enabled.

-- 
H.J.

Re: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with makecontext family functions

[Jakub Jelinek]

On Tue, Dec 19, 2017 at 09:42:11AM +, Tsimbalist, Igor V wrote:
> > I don't see any discussion in the bugzilla issue to explain this.
> 
> This option is needed to support  two cases:
> 
> 1. Compilation of ucontext functions inside glibc. To have glibc itself be CET
> compatible all files comprises the library has to be CET compatible. That 
> means
> the module with ucontext functions from glibc has to be forced to be CET
> compatible.
> 
> 2. Compilation of a user application with ucontext functions. In this case the
> error has to be issued, so no usage of a ucontext functions.
> 
> Having just __SHSTK__ macro it's impossible to handle both cases. The case
> #1 will report an error during glibc compilation. A new macro is introduced to
> use in the source to handle these cases. To control the value of the new macro
> a new option is introduced.

That doesn't make sense.  Just add

#if defined(__SHSTK__) && !defined(_LIBC)
#define __UCONTEXT_UNSUPPORTED \
  __attribute__((__error__ ("-mshstk incompatible with ucontext.h APIs")))
#else
#define __UCONTEXT_UNSUPPORTED
#endif

and use __UCONTEXT_UNSUPPORTED on the 4 function declarations, you'll be
able to compile glibc itself, but not something against it.

Have you considered implementing these 4 functions or some helper they could
use in the kernel if CET is enabled and doing there with the shadow stack
whatever is needed?

Jakub

RE: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with makecontext family functions

[Tsimbalist, Igor V]

> -Original Message-
> From: Jeff Law [mailto:l...@redhat.com]
> Sent: Tuesday, December 19, 2017 6:15 AM
> To: Sandra Loosemore <san...@codesourcery.com>; Tsimbalist, Igor V
> <igor.v.tsimbal...@intel.com>; gcc-patches@gcc.gnu.org
> Cc: Uros Bizjak <ubiz...@gmail.com>
> Subject: Re: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with
> makecontext family functions
> 
> On 12/18/2017 12:39 PM, Sandra Loosemore wrote:
> > On 12/17/2017 05:05 PM, Tsimbalist, Igor V wrote:
> >> -fcf-protection -mcet is incompatible with makecontext family functions
> >> since they can't properly set up and destroy shadow stack pointer. This
> >> change provides a mechanism to help detection shadow stack
> compatibility.
> >> The current proposal is to add -mcheck-shstk-compat option which will
> >> predefine __CHECK_SHSTK_COMPAT__ macro. The option will be
> >> set on by default.  Then we can add a code
> >>
> >> #if defined __SHSTK__ && defined __CHECK_SHSTK_COMPAT__
> >> # error This source is incompatible with -mshstk
> >> #endif
> >>
> >> to .
> >
> > The functional change here is out of my maintainership domain, but
> > Why does this need a new macro and a new option to control it?  If the
> > code being protected doesn't work properly with -mshstk, it seems like
> > it would be more robust to do just
> >
> > #if defined __SHSTK__
> > # error This source is incompatible with -mshstk
> > #endif
> >
> > I don't see any discussion in the bugzilla issue to explain this.
> I'd tend to agree.  Making another option to handle this seems excessive.
I replied to Sandra's email and updated the bugzilla issue.

Igor

> jeff

RE: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with makecontext family functions

[Tsimbalist, Igor V]

> -Original Message-
> From: Sandra Loosemore [mailto:san...@codesourcery.com]
> Sent: Monday, December 18, 2017 8:39 PM
> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; gcc-
> patc...@gcc.gnu.org
> Cc: Uros Bizjak <ubiz...@gmail.com>
> Subject: Re: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with
> makecontext family functions
> 
> On 12/17/2017 05:05 PM, Tsimbalist, Igor V wrote:
> > -fcf-protection -mcet is incompatible with makecontext family functions
> > since they can't properly set up and destroy shadow stack pointer. This
> > change provides a mechanism to help detection shadow stack
> compatibility.
> > The current proposal is to add -mcheck-shstk-compat option which will
> > predefine __CHECK_SHSTK_COMPAT__ macro. The option will be
> > set on by default.  Then we can add a code
> >
> > #if defined __SHSTK__ && defined __CHECK_SHSTK_COMPAT__
> > # error This source is incompatible with -mshstk
> > #endif
> >
> > to .
> 
> The functional change here is out of my maintainership domain, but
> Why does this need a new macro and a new option to control it?  If the
> code being protected doesn't work properly with -mshstk, it seems like
> it would be more robust to do just
> 
> #if defined __SHSTK__
> # error This source is incompatible with -mshstk
> #endif
> 
> I don't see any discussion in the bugzilla issue to explain this.

This option is needed to support  two cases:

1. Compilation of ucontext functions inside glibc. To have glibc itself be CET
compatible all files comprises the library has to be CET compatible. That means
the module with ucontext functions from glibc has to be forced to be CET
compatible.

2. Compilation of a user application with ucontext functions. In this case the
error has to be issued, so no usage of a ucontext functions.

Having just __SHSTK__ macro it's impossible to handle both cases. The case
#1 will report an error during glibc compilation. A new macro is introduced to
use in the source to handle these cases. To control the value of the new macro
a new option is introduced.

A separate discussion was started by HJ to what is the best way to handle
ucontext functions in presence of CET. The proposal is to introduce a new 
interface,
that can properly handle shadow stack (but this it out of this PR).

I have added this info to the bugzilla issue.

> Re the proposed documentation for the new option:
> 
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index 1413095..7b4223a 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -26225,6 +26225,15 @@ The option has effect only if the @option{-fcf-
> protection=full} or
> >  @option{-mshstk} is on by default when the @option{-mcet} option is
> >  specified.
> >
> > +@item -mcheck-shstk-compat
> > +@opindex mcheck-shstk-compat
> > +This option predefines __CHECK_SHSTK_COMPAT__ macro, which can be
> used
> 
> You need to add @code markup on all the macro names here.
Done.

> > +to add a guard to the C/C++ sources which are incompatible with Intel
> 
> s/which/that/
Fixed.

Igor

> > +shadow stack technology.  A typical case would be issuing an error when >
> +both __SHSTK__ and __CHECK_SHSTK_COMPAT__ macro are defined.  The
> option
> > +@option{-mcheck-shstk-compat} is on by default when the @code{-
> mshstk}
> > +option is specified.
> > +
> >  @item -mcrc32
> >  @opindex mcrc32
> >  This option enables built-in functions @code{__builtin_ia32_crc32qi},
> 
> -Sandra

Re: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with makecontext family functions

[Jeff Law]

On 12/18/2017 12:39 PM, Sandra Loosemore wrote:
> On 12/17/2017 05:05 PM, Tsimbalist, Igor V wrote:
>> -fcf-protection -mcet is incompatible with makecontext family functions
>> since they can't properly set up and destroy shadow stack pointer. This
>> change provides a mechanism to help detection shadow stack compatibility.
>> The current proposal is to add -mcheck-shstk-compat option which will
>> predefine __CHECK_SHSTK_COMPAT__ macro. The option will be
>> set on by default.  Then we can add a code
>>
>> #if defined __SHSTK__ && defined __CHECK_SHSTK_COMPAT__
>> # error This source is incompatible with -mshstk
>> #endif
>>
>> to .
> 
> The functional change here is out of my maintainership domain, but
> Why does this need a new macro and a new option to control it?  If the
> code being protected doesn't work properly with -mshstk, it seems like
> it would be more robust to do just
> 
> #if defined __SHSTK__
> # error This source is incompatible with -mshstk
> #endif
> 
> I don't see any discussion in the bugzilla issue to explain this.
I'd tend to agree.  Making another option to handle this seems excessive.

jeff

Re: [i386] PR81842 [CET] -fcf-protection -mcet is incompatible with makecontext family functions

[Sandra Loosemore]

On 12/17/2017 05:05 PM, Tsimbalist, Igor V wrote:

-fcf-protection -mcet is incompatible with makecontext family functions
since they can't properly set up and destroy shadow stack pointer. This
change provides a mechanism to help detection shadow stack compatibility.
The current proposal is to add -mcheck-shstk-compat option which will
predefine __CHECK_SHSTK_COMPAT__ macro. The option will be
set on by default.  Then we can add a code

#if defined __SHSTK__ && defined __CHECK_SHSTK_COMPAT__
# error This source is incompatible with -mshstk
#endif

to .


The functional change here is out of my maintainership domain, but
Why does this need a new macro and a new option to control it?  If the 
code being protected doesn't work properly with -mshstk, it seems like 
it would be more robust to do just


#if defined __SHSTK__
# error This source is incompatible with -mshstk
#endif

I don't see any discussion in the bugzilla issue to explain this.

Re the proposed documentation for the new option:


diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 1413095..7b4223a 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -26225,6 +26225,15 @@ The option has effect only if the 
@option{-fcf-protection=full} or
 @option{-mshstk} is on by default when the @option{-mcet} option is
 specified.
 
+@item -mcheck-shstk-compat

+@opindex mcheck-shstk-compat
+This option predefines __CHECK_SHSTK_COMPAT__ macro, which can be used


You need to add @code markup on all the macro names here.


+to add a guard to the C/C++ sources which are incompatible with Intel


s/which/that/


+shadow stack technology.  A typical case would be issuing an error when > 
+both __SHSTK__ and __CHECK_SHSTK_COMPAT__ macro are defined.  The option
+@option{-mcheck-shstk-compat} is on by default when the @code{-mshstk}
+option is specified.
+
 @item -mcrc32
 @opindex mcrc32
 This option enables built-in functions @code{__builtin_ia32_crc32qi},


-Sandra