| Message ID | 20180718153752.GB13951@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Call REAL(swapcontext) with indirect_return attribute on x86 | expand |
What's ENDBR and do we really need to have it in compiler-rt? As usual, I am opposed to any gcc compiler-rt that bypass upstream. --kcc On Wed, Jul 18, 2018 at 8:37 AM H.J. Lu <hongjiu.lu@intel.com> wrote: > > asan/asan_interceptors.cc has > > ... > int res = REAL(swapcontext)(oucp, ucp); > ... > > REAL(swapcontext) is a function pointer to swapcontext in libc. Since > swapcontext may return via indirect branch on x86 when shadow stack is > enabled, we need to call REAL(swapcontext) with indirect_return attribute > on x86 so that compiler can insert ENDBR after REAL(swapcontext) call. > > I opened an LLVM bug: > > https://bugs.llvm.org/show_bug.cgi?id=38207 > > But it won't get fixed before indirect_return attribute is added to > LLVM. I'd like to get it fixed in GCC first. > > Tested on i386 and x86-64. OK for trunk after > > https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01007.html > > is approved? > > Thanks. > > > H.J. > --- > PR target/86560 > * asan/asan_interceptors.cc (swapcontext): Call REAL(swapcontext) > with indirect_return attribute on x86. > --- > libsanitizer/asan/asan_interceptors.cc | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/libsanitizer/asan/asan_interceptors.cc b/libsanitizer/asan/asan_interceptors.cc > index a8f4b72723f..b8dde4f19c5 100644 > --- a/libsanitizer/asan/asan_interceptors.cc > +++ b/libsanitizer/asan/asan_interceptors.cc > @@ -267,7 +267,13 @@ INTERCEPTOR(int, swapcontext, struct ucontext_t *oucp, > uptr stack, ssize; > ReadContextStack(ucp, &stack, &ssize); > ClearShadowMemoryForContextStack(stack, ssize); > +#if defined(__x86_64__) || defined(__i386__) > + int (*real_swapcontext) (struct ucontext_t *, struct ucontext_t *) > + __attribute__((__indirect_return__)) = REAL(swapcontext); > + int res = real_swapcontext(oucp, ucp); > +#else > int res = REAL(swapcontext)(oucp, ucp); > +#endif > // swapcontext technically does not return, but program may swap context to > // "oucp" later, that would look as if swapcontext() returned 0. > // We need to clear shadow for ucp once again, as it may be in arbitrary > -- > 2.17.1 >
On Wed, Jul 18, 2018 at 11:18 AM, Kostya Serebryany <kcc@google.com> wrote: > What's ENDBR and do we really need to have it in compiler-rt? When shadow stack from Intel CET is enabled, the first instruction of all indirect branch targets must be a special instruction, ENDBR. In this case, int res = REAL(swapcontext)(oucp, ucp); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This function may be returned via an indirect branch. Here compiler must insert ENDBR after call, like call *bar(%rip) endbr64 > As usual, I am opposed to any gcc compiler-rt that bypass upstream. We want it to be fixed in upstream. That is why I opened an LLVM bug. > --kcc > > On Wed, Jul 18, 2018 at 8:37 AM H.J. Lu <hongjiu.lu@intel.com> wrote: >> >> asan/asan_interceptors.cc has >> >> ... >> int res = REAL(swapcontext)(oucp, ucp); >> ... >> >> REAL(swapcontext) is a function pointer to swapcontext in libc. Since >> swapcontext may return via indirect branch on x86 when shadow stack is >> enabled, we need to call REAL(swapcontext) with indirect_return attribute >> on x86 so that compiler can insert ENDBR after REAL(swapcontext) call. >> >> I opened an LLVM bug: >> >> https://bugs.llvm.org/show_bug.cgi?id=38207 >> >> But it won't get fixed before indirect_return attribute is added to >> LLVM. I'd like to get it fixed in GCC first. >> >> Tested on i386 and x86-64. OK for trunk after >> >> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01007.html >> >> is approved? >> >> Thanks. >> >> >> H.J. >> --- >> PR target/86560 >> * asan/asan_interceptors.cc (swapcontext): Call REAL(swapcontext) >> with indirect_return attribute on x86. >> --- >> libsanitizer/asan/asan_interceptors.cc | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/libsanitizer/asan/asan_interceptors.cc b/libsanitizer/asan/asan_interceptors.cc >> index a8f4b72723f..b8dde4f19c5 100644 >> --- a/libsanitizer/asan/asan_interceptors.cc >> +++ b/libsanitizer/asan/asan_interceptors.cc >> @@ -267,7 +267,13 @@ INTERCEPTOR(int, swapcontext, struct ucontext_t *oucp, >> uptr stack, ssize; >> ReadContextStack(ucp, &stack, &ssize); >> ClearShadowMemoryForContextStack(stack, ssize); >> +#if defined(__x86_64__) || defined(__i386__) >> + int (*real_swapcontext) (struct ucontext_t *, struct ucontext_t *) >> + __attribute__((__indirect_return__)) = REAL(swapcontext); >> + int res = real_swapcontext(oucp, ucp); >> +#else >> int res = REAL(swapcontext)(oucp, ucp); >> +#endif >> // swapcontext technically does not return, but program may swap context to >> // "oucp" later, that would look as if swapcontext() returned 0. >> // We need to clear shadow for ucp once again, as it may be in arbitrary >> -- >> 2.17.1 >>
On Wed, Jul 18, 2018 at 11:40 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Wed, Jul 18, 2018 at 11:18 AM, Kostya Serebryany <kcc@google.com> wrote: > > What's ENDBR and do we really need to have it in compiler-rt? > > When shadow stack from Intel CET is enabled, the first instruction of all > indirect branch targets must be a special instruction, ENDBR. In this case, I am confused. CET is a security mitigation feature (and ENDBR is a pretty weak form of such), while ASAN is a testing tool, rarely used in production is almost never as a mitigation (which it is not!). Why would anyone need to combine CET and ASAN in one process? Also, CET doesn't exist in the hardware yet, at least not publicly available. Which means there should be no rush (am I wrong?) and we can do things in the correct order: implement the Clang/LLVM support, make the compiler-rt change in LLVM, merge back to GCC. --kcc > > int res = REAL(swapcontext)(oucp, ucp); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This function may be > returned via an indirect branch. > > Here compiler must insert ENDBR after call, like > > call *bar(%rip) > endbr64 > > > As usual, I am opposed to any gcc compiler-rt that bypass upstream. > > We want it to be fixed in upstream. That is why I opened an LLVM bug. > > > > --kcc > > > > On Wed, Jul 18, 2018 at 8:37 AM H.J. Lu <hongjiu.lu@intel.com> wrote: > >> > >> asan/asan_interceptors.cc has > >> > >> ... > >> int res = REAL(swapcontext)(oucp, ucp); > >> ... > >> > >> REAL(swapcontext) is a function pointer to swapcontext in libc. Since > >> swapcontext may return via indirect branch on x86 when shadow stack is > >> enabled, we need to call REAL(swapcontext) with indirect_return attribute > >> on x86 so that compiler can insert ENDBR after REAL(swapcontext) call. > >> > >> I opened an LLVM bug: > >> > >> https://bugs.llvm.org/show_bug.cgi?id=38207 > >> > >> But it won't get fixed before indirect_return attribute is added to > >> LLVM. I'd like to get it fixed in GCC first. > >> > >> Tested on i386 and x86-64. OK for trunk after > >> > >> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01007.html > >> > >> is approved? > >> > >> Thanks. > >> > >> > >> H.J. > >> --- > >> PR target/86560 > >> * asan/asan_interceptors.cc (swapcontext): Call REAL(swapcontext) > >> with indirect_return attribute on x86. > >> --- > >> libsanitizer/asan/asan_interceptors.cc | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/libsanitizer/asan/asan_interceptors.cc b/libsanitizer/asan/asan_interceptors.cc > >> index a8f4b72723f..b8dde4f19c5 100644 > >> --- a/libsanitizer/asan/asan_interceptors.cc > >> +++ b/libsanitizer/asan/asan_interceptors.cc > >> @@ -267,7 +267,13 @@ INTERCEPTOR(int, swapcontext, struct ucontext_t *oucp, > >> uptr stack, ssize; > >> ReadContextStack(ucp, &stack, &ssize); > >> ClearShadowMemoryForContextStack(stack, ssize); > >> +#if defined(__x86_64__) || defined(__i386__) > >> + int (*real_swapcontext) (struct ucontext_t *, struct ucontext_t *) > >> + __attribute__((__indirect_return__)) = REAL(swapcontext); > >> + int res = real_swapcontext(oucp, ucp); > >> +#else > >> int res = REAL(swapcontext)(oucp, ucp); > >> +#endif > >> // swapcontext technically does not return, but program may swap context to > >> // "oucp" later, that would look as if swapcontext() returned 0. > >> // We need to clear shadow for ucp once again, as it may be in arbitrary > >> -- > >> 2.17.1 > >> > > > > -- > H.J.
On Wed, Jul 18, 2018 at 11:45 AM, Kostya Serebryany <kcc@google.com> wrote: > On Wed, Jul 18, 2018 at 11:40 AM H.J. Lu <hjl.tools@gmail.com> wrote: >> >> On Wed, Jul 18, 2018 at 11:18 AM, Kostya Serebryany <kcc@google.com> wrote: >> > What's ENDBR and do we really need to have it in compiler-rt? >> >> When shadow stack from Intel CET is enabled, the first instruction of all >> indirect branch targets must be a special instruction, ENDBR. In this case, > > I am confused. > CET is a security mitigation feature (and ENDBR is a pretty weak form of such), > while ASAN is a testing tool, rarely used in production is almost > never as a mitigation (which it is not!). > Why would anyone need to combine CET and ASAN in one process? > CET is transparent to ASAN. It is perfectly OK to use -fcf-protection to enable CET together with ASAN. > Also, CET doesn't exist in the hardware yet, at least not publicly available. > Which means there should be no rush (am I wrong?) and we can do things > in the correct order: > implement the Clang/LLVM support, make the compiler-rt change in LLVM, > merge back to GCC. I am working with our LLVM people to address this. H.J. > --kcc > >> >> int res = REAL(swapcontext)(oucp, ucp); >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This function may be >> returned via an indirect branch. >> >> Here compiler must insert ENDBR after call, like >> >> call *bar(%rip) >> endbr64 >> >> > As usual, I am opposed to any gcc compiler-rt that bypass upstream. >> >> We want it to be fixed in upstream. That is why I opened an LLVM bug. >> >> >> > --kcc >> > >> > On Wed, Jul 18, 2018 at 8:37 AM H.J. Lu <hongjiu.lu@intel.com> wrote: >> >> >> >> asan/asan_interceptors.cc has >> >> >> >> ... >> >> int res = REAL(swapcontext)(oucp, ucp); >> >> ... >> >> >> >> REAL(swapcontext) is a function pointer to swapcontext in libc. Since >> >> swapcontext may return via indirect branch on x86 when shadow stack is >> >> enabled, we need to call REAL(swapcontext) with indirect_return attribute >> >> on x86 so that compiler can insert ENDBR after REAL(swapcontext) call. >> >> >> >> I opened an LLVM bug: >> >> >> >> https://bugs.llvm.org/show_bug.cgi?id=38207 >> >> >> >> But it won't get fixed before indirect_return attribute is added to >> >> LLVM. I'd like to get it fixed in GCC first. >> >> >> >> Tested on i386 and x86-64. OK for trunk after >> >> >> >> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01007.html >> >> >> >> is approved? >> >> >> >> Thanks. >> >> >> >> >> >> H.J. >> >> --- >> >> PR target/86560 >> >> * asan/asan_interceptors.cc (swapcontext): Call REAL(swapcontext) >> >> with indirect_return attribute on x86. >> >> --- >> >> libsanitizer/asan/asan_interceptors.cc | 6 ++++++ >> >> 1 file changed, 6 insertions(+) >> >> >> >> diff --git a/libsanitizer/asan/asan_interceptors.cc b/libsanitizer/asan/asan_interceptors.cc >> >> index a8f4b72723f..b8dde4f19c5 100644 >> >> --- a/libsanitizer/asan/asan_interceptors.cc >> >> +++ b/libsanitizer/asan/asan_interceptors.cc >> >> @@ -267,7 +267,13 @@ INTERCEPTOR(int, swapcontext, struct ucontext_t *oucp, >> >> uptr stack, ssize; >> >> ReadContextStack(ucp, &stack, &ssize); >> >> ClearShadowMemoryForContextStack(stack, ssize); >> >> +#if defined(__x86_64__) || defined(__i386__) >> >> + int (*real_swapcontext) (struct ucontext_t *, struct ucontext_t *) >> >> + __attribute__((__indirect_return__)) = REAL(swapcontext); >> >> + int res = real_swapcontext(oucp, ucp); >> >> +#else >> >> int res = REAL(swapcontext)(oucp, ucp); >> >> +#endif >> >> // swapcontext technically does not return, but program may swap context to >> >> // "oucp" later, that would look as if swapcontext() returned 0. >> >> // We need to clear shadow for ucp once again, as it may be in arbitrary >> >> -- >> >> 2.17.1 >> >> >> >> >> >> -- >> H.J.
On Wed, Jul 18, 2018 at 12:29 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Wed, Jul 18, 2018 at 11:45 AM, Kostya Serebryany <kcc@google.com> wrote: > > On Wed, Jul 18, 2018 at 11:40 AM H.J. Lu <hjl.tools@gmail.com> wrote: > >> > >> On Wed, Jul 18, 2018 at 11:18 AM, Kostya Serebryany <kcc@google.com> wrote: > >> > What's ENDBR and do we really need to have it in compiler-rt? > >> > >> When shadow stack from Intel CET is enabled, the first instruction of all > >> indirect branch targets must be a special instruction, ENDBR. In this case, > > > > I am confused. > > CET is a security mitigation feature (and ENDBR is a pretty weak form of such), > > while ASAN is a testing tool, rarely used in production is almost > > never as a mitigation (which it is not!). > > Why would anyone need to combine CET and ASAN in one process? > > > > CET is transparent to ASAN. It is perfectly OK to use -fcf-protection to > enable CET together with ASAN. It is ok, but does it make any sense? If anything, the current ASAN's intereceptors are a large blob of security vulnerabilities. If we ever want to use ASAN (or, more likely, HWASAN) as a security mitigation feature, we will need to get rid of these interceptors entirely. > > > Also, CET doesn't exist in the hardware yet, at least not publicly available. > > Which means there should be no rush (am I wrong?) and we can do things > > in the correct order: > > implement the Clang/LLVM support, make the compiler-rt change in LLVM, > > merge back to GCC. > > I am working with our LLVM people to address this. Cool! > > H.J. > > --kcc > > > >> > >> int res = REAL(swapcontext)(oucp, ucp); > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This function may be > >> returned via an indirect branch. > >> > >> Here compiler must insert ENDBR after call, like > >> > >> call *bar(%rip) > >> endbr64 > >> > >> > As usual, I am opposed to any gcc compiler-rt that bypass upstream. > >> > >> We want it to be fixed in upstream. That is why I opened an LLVM bug. > >> > >> > >> > --kcc > >> > > >> > On Wed, Jul 18, 2018 at 8:37 AM H.J. Lu <hongjiu.lu@intel.com> wrote: > >> >> > >> >> asan/asan_interceptors.cc has > >> >> > >> >> ... > >> >> int res = REAL(swapcontext)(oucp, ucp); > >> >> ... > >> >> > >> >> REAL(swapcontext) is a function pointer to swapcontext in libc. Since > >> >> swapcontext may return via indirect branch on x86 when shadow stack is > >> >> enabled, we need to call REAL(swapcontext) with indirect_return attribute > >> >> on x86 so that compiler can insert ENDBR after REAL(swapcontext) call. > >> >> > >> >> I opened an LLVM bug: > >> >> > >> >> https://bugs.llvm.org/show_bug.cgi?id=38207 > >> >> > >> >> But it won't get fixed before indirect_return attribute is added to > >> >> LLVM. I'd like to get it fixed in GCC first. > >> >> > >> >> Tested on i386 and x86-64. OK for trunk after > >> >> > >> >> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01007.html > >> >> > >> >> is approved? > >> >> > >> >> Thanks. > >> >> > >> >> > >> >> H.J. > >> >> --- > >> >> PR target/86560 > >> >> * asan/asan_interceptors.cc (swapcontext): Call REAL(swapcontext) > >> >> with indirect_return attribute on x86. > >> >> --- > >> >> libsanitizer/asan/asan_interceptors.cc | 6 ++++++ > >> >> 1 file changed, 6 insertions(+) > >> >> > >> >> diff --git a/libsanitizer/asan/asan_interceptors.cc b/libsanitizer/asan/asan_interceptors.cc > >> >> index a8f4b72723f..b8dde4f19c5 100644 > >> >> --- a/libsanitizer/asan/asan_interceptors.cc > >> >> +++ b/libsanitizer/asan/asan_interceptors.cc > >> >> @@ -267,7 +267,13 @@ INTERCEPTOR(int, swapcontext, struct ucontext_t *oucp, > >> >> uptr stack, ssize; > >> >> ReadContextStack(ucp, &stack, &ssize); > >> >> ClearShadowMemoryForContextStack(stack, ssize); > >> >> +#if defined(__x86_64__) || defined(__i386__) > >> >> + int (*real_swapcontext) (struct ucontext_t *, struct ucontext_t *) > >> >> + __attribute__((__indirect_return__)) = REAL(swapcontext); > >> >> + int res = real_swapcontext(oucp, ucp); > >> >> +#else > >> >> int res = REAL(swapcontext)(oucp, ucp); > >> >> +#endif > >> >> // swapcontext technically does not return, but program may swap context to > >> >> // "oucp" later, that would look as if swapcontext() returned 0. > >> >> // We need to clear shadow for ucp once again, as it may be in arbitrary > >> >> -- > >> >> 2.17.1 > >> >> > >> > >> > >> > >> -- > >> H.J. > > > > -- > H.J.
On Wed, Jul 18, 2018 at 12:34:28PM -0700, Kostya Serebryany wrote: > On Wed, Jul 18, 2018 at 12:29 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > On Wed, Jul 18, 2018 at 11:45 AM, Kostya Serebryany <kcc@google.com> wrote: > > > On Wed, Jul 18, 2018 at 11:40 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > >> > > >> On Wed, Jul 18, 2018 at 11:18 AM, Kostya Serebryany <kcc@google.com> wrote: > > >> > What's ENDBR and do we really need to have it in compiler-rt? > > >> > > >> When shadow stack from Intel CET is enabled, the first instruction of all > > >> indirect branch targets must be a special instruction, ENDBR. In this case, > > > > > > I am confused. > > > CET is a security mitigation feature (and ENDBR is a pretty weak form of such), > > > while ASAN is a testing tool, rarely used in production is almost > > > never as a mitigation (which it is not!). > > > Why would anyone need to combine CET and ASAN in one process? > > > > > > > CET is transparent to ASAN. It is perfectly OK to use -fcf-protection to > > enable CET together with ASAN. > > It is ok, but does it make any sense? > If anything, the current ASAN's intereceptors are a large blob of > security vulnerabilities. > If we ever want to use ASAN (or, more likely, HWASAN) as a security > mitigation feature, > we will need to get rid of these interceptors entirely. > > > > > > > Also, CET doesn't exist in the hardware yet, at least not publicly available. > > > Which means there should be no rush (am I wrong?) and we can do things > > > in the correct order: > > > implement the Clang/LLVM support, make the compiler-rt change in LLVM, > > > merge back to GCC. > > > > I am working with our LLVM people to address this. > > Cool! > I am testing this patch and will submit it upstream. H.J. --- asan/asan_interceptors.cc has ... int res = REAL(swapcontext)(oucp, ucp); ... REAL(swapcontext) is a function pointer to swapcontext in libc. Since swapcontext may return via indirect branch on x86 when shadow stack is enabled, we need to call REAL(swapcontext) with indirect_return attribute on x86 so that compiler can insert ENDBR after REAL(swapcontext) call. PR target/86560 * asan/asan_interceptors.cc (swapcontext): Call REAL(swapcontext) with indirect_return attribute on x86 if indirect_return attribute is available. --- libsanitizer/asan/asan_interceptors.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/libsanitizer/asan/asan_interceptors.cc b/libsanitizer/asan/asan_interceptors.cc index a8f4b72723f..3ae473f210a 100644 --- a/libsanitizer/asan/asan_interceptors.cc +++ b/libsanitizer/asan/asan_interceptors.cc @@ -267,7 +267,16 @@ INTERCEPTOR(int, swapcontext, struct ucontext_t *oucp, uptr stack, ssize; ReadContextStack(ucp, &stack, &ssize); ClearShadowMemoryForContextStack(stack, ssize); +#if defined(__has_attribute) && (defined(__x86_64__) || defined(__i386__)) + int (*real_swapcontext) (struct ucontext_t *, struct ucontext_t *) +# if __has_attribute (__indirect_return__) + __attribute__((__indirect_return__)) +# endif + = REAL(swapcontext); + int res = real_swapcontext(oucp, ucp); +#else int res = REAL(swapcontext)(oucp, ucp); +#endif // swapcontext technically does not return, but program may swap context to // "oucp" later, that would look as if swapcontext() returned 0. // We need to clear shadow for ucp once again, as it may be in arbitrary
diff --git a/libsanitizer/asan/asan_interceptors.cc b/libsanitizer/asan/asan_interceptors.cc index a8f4b72723f..b8dde4f19c5 100644 --- a/libsanitizer/asan/asan_interceptors.cc +++ b/libsanitizer/asan/asan_interceptors.cc @@ -267,7 +267,13 @@ INTERCEPTOR(int, swapcontext, struct ucontext_t *oucp, uptr stack, ssize; ReadContextStack(ucp, &stack, &ssize); ClearShadowMemoryForContextStack(stack, ssize); +#if defined(__x86_64__) || defined(__i386__) + int (*real_swapcontext) (struct ucontext_t *, struct ucontext_t *) + __attribute__((__indirect_return__)) = REAL(swapcontext); + int res = real_swapcontext(oucp, ucp); +#else int res = REAL(swapcontext)(oucp, ucp); +#endif // swapcontext technically does not return, but program may swap context to // "oucp" later, that would look as if swapcontext() returned 0. // We need to clear shadow for ucp once again, as it may be in arbitrary