This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Call REAL(swapcontext) with indirect_return attribute on x86


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]