Message ID | CAMe9rOp_b2vtkJqc=0vf1rzk2==e3YA4W4Dvkp=FqopmswfoLg@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 8008 invoked by alias); 7 Mar 2018 22:07:00 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 7989 invoked by uid 89); 7 Mar 2018 22:07:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.1 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=Exclude X-HELO: mail-ot0-f196.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=djei8V03SmPHfaievjfxotdDnXsFhkDzxkJ0ThSL+3E=; b=esU0mI/yOPD5sO4YkdS6W2dNJ6HCN4/TjqNaTedvBWwAZDcx4AK+End88cL+0JxlmR B2S1wLW7ga21e3R1/lHTHHKLBJVyfpxdtSpsc/hlBQ+mdYf3Oftvg2F3H0zUv+IOmRM6 1ormW87RpBHmG9JTTZlxRuknHKtAkdFRLiOgs+XFZytcB/s1Ne65x3Y5YFD0FbvmsWYN OpPVmsJxfD+rbJTENYXpYc6+A3/sKxwOCeIkeids7prBHQaRWvIz16ddZTMzovudJBvg wUIuYWwWY/BZc5LvfQYZJMW/Njb1kD83LM6cNPz5qNng2+ZlZ+2T6UvsYDq2sC3bDqXZ DSXg== X-Gm-Message-State: AElRT7E+OHrxTFRhIS/7XuiSWfJZVNd4Whc/tT8hZ/h7NGM4w3VOB9a+ R1vkFVbRhJyzt0UMaDH7eLG57tGHOwtRC3Xj//w= X-Google-Smtp-Source: AG47ELv0K6Qsqfooc+zgR+MVVVkqJ0vILpIASCE8qAngONiRyjp9ad5+zMAqCVFPLbhVHnFYZIVULkSk17gF2gBcJCg= X-Received: by 10.157.82.104 with SMTP id q40mr17316320otg.332.1520460416886; Wed, 07 Mar 2018 14:06:56 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <CAMe9rOpVQ9oEK0WYdPvR_1nuvkpwXy3Rsy5UYsXvOnScRu2toA@mail.gmail.com> References: <20180201205757.51911-1-hjl.tools@gmail.com> <87a7vyjsqv.fsf@mid.deneb.enyo.de> <CAMe9rOrkNQudxKwPmrOkrWD7URO+mzdOeQhNqMns2a2QSq0S7g@mail.gmail.com> <87vaelbetu.fsf@mid.deneb.enyo.de> <CAMe9rOqCPk55Ke9T+0194myAiQHwKZVv=gzm3pHDtULTaT0S3A@mail.gmail.com> <87fu5pb7ql.fsf@mid.deneb.enyo.de> <CAMe9rOofbxtenVEP8sQSi4-0TAn6gsRBWi88RyzchwaxvEtGeA@mail.gmail.com> <CAMe9rOqowauyuyvJg5sjktTJYkNJ3NcvXzLQ7mz_+5F9AXQrWw@mail.gmail.com> <877er1b4zp.fsf@mid.deneb.enyo.de> <CAMe9rOrZaz2RbcHBZ8MSjx0VaLpNxd=8RhmJoXmH0fps6qTYHA@mail.gmail.com> <87371pb3ga.fsf@mid.deneb.enyo.de> <CAMe9rOqPkq7gQ1e6EE-87kVuBnMZrqmOeNExQ1ZmGsqufZu3MQ@mail.gmail.com> <87tvu59o21.fsf@mid.deneb.enyo.de> <CAMe9rOqRz=YR78QqD_EqAagZ3yr1v8jqJvn2WuF=8KCJp9csfw@mail.gmail.com> <87po4t9mxt.fsf@mid.deneb.enyo.de> <CAMe9rOpcDM0hp_h1EomZrjaR2raSiS5kB-B0o6fFRBZ9ysA=Ew@mail.gmail.com> <3764b0a1-9f26-6f5f-1bc5-d374f2672f3a@redhat.com> <CAMe9rOrA+yv2E8u0NjAMMhPkpL5+rk6TuhCj7dhBiupryUGQ7w@mail.gmail.com> <86d5d5ba-2b53-1904-dada-3efe2b3ad501@redhat.com> <CAMe9rOqGDo4SpTQ=BBRK+RJcVVN3M9WQxwWFJjK+k=+nvTPAOg@mail.gmail.com> <CAMe9rOpVQ9oEK0WYdPvR_1nuvkpwXy3Rsy5UYsXvOnScRu2toA@mail.gmail.com> From: "H.J. Lu" <hjl.tools@gmail.com> Date: Wed, 7 Mar 2018 14:06:56 -0800 Message-ID: <CAMe9rOp_b2vtkJqc=0vf1rzk2==e3YA4W4Dvkp=FqopmswfoLg@mail.gmail.com> Subject: Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf To: "Carlos O'Donell" <carlos@redhat.com>, "Tsimbalist, Igor V" <igor.v.tsimbalist@intel.com> Cc: Florian Weimer <fw@deneb.enyo.de>, GNU C Library <libc-alpha@sourceware.org> Content-Type: text/plain; charset="UTF-8" |
Commit Message
H.J. Lu
March 7, 2018, 10:06 p.m. UTC
On Wed, Mar 7, 2018 at 12:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com> wrote: >>> On 03/07/2018 03:56 AM, H.J. Lu wrote: >>>>>> 1. I have to add __setjmp_cancel and __sigsetjmp_cancel which won't >>>>>> save and restore shadow stack register. >>>> >>>> I have been testing this. I ran into one issue. GCC knows that setjmp will >>>> return via longjmp and inserts ENDBR after it. But it doesn't know >>>> __setjmp_cancel and __sigsetjmp_cancel. We can either add them to GCC >>>> or add NOTRACK prefix to the corresponding longjmps. >>> >>> I would rather GCC did not know about these implementation details. >>> >>> I have no objection to the NOTRACK prefix in the corresponding longjmps. >>> >>> What would be a downside to this choice? >>> >> >> NOTRACK prefix is typically generated by compiler for switch table. Compiler >> knows each indirect jump target is valid and pointer load for indirect jump is >> generated by compiler in read-only section. This is pretty safe since there is >> very little chance for malicious codes to temper the pointer value. However, >> in case of longjmp, the indirect jump target is in jmpbuf. There is >> a possilibty >> for malicious codes to change the indirect jump target such that longjmp wil >> jump to the wrong place. Use NOTRACK prefix here defeats the purpose of >> indirect branch tracking in CET. >> > > Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may > return twice, similar to setjmp. > Here is the GCC patch:
Comments
> -----Original Message----- > From: H.J. Lu [mailto:hjl.tools@gmail.com] > Sent: Wednesday, March 7, 2018 11:07 PM > To: Carlos O'Donell <carlos@redhat.com>; Tsimbalist, Igor V > <igor.v.tsimbalist@intel.com> > Cc: Florian Weimer <fw@deneb.enyo.de>; GNU C Library <libc- > alpha@sourceware.org> > Subject: Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf > > On Wed, Mar 7, 2018 at 12:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > > On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > >> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com> > wrote: > >>> On 03/07/2018 03:56 AM, H.J. Lu wrote: > >>>>>> 1. I have to add __setjmp_cancel and __sigsetjmp_cancel which > won't > >>>>>> save and restore shadow stack register. > >>>> > >>>> I have been testing this. I ran into one issue. GCC knows that setjmp > will > >>>> return via longjmp and inserts ENDBR after it. But it doesn't know > >>>> __setjmp_cancel and __sigsetjmp_cancel. We can either add them to > GCC > >>>> or add NOTRACK prefix to the corresponding longjmps. > >>> > >>> I would rather GCC did not know about these implementation details. > >>> > >>> I have no objection to the NOTRACK prefix in the corresponding > longjmps. > >>> > >>> What would be a downside to this choice? > >>> > >> > >> NOTRACK prefix is typically generated by compiler for switch table. > Compiler > >> knows each indirect jump target is valid and pointer load for indirect > jump is > >> generated by compiler in read-only section. This is pretty safe since there > is > >> very little chance for malicious codes to temper the pointer value. > However, > >> in case of longjmp, the indirect jump target is in jmpbuf. There is > >> a possilibty > >> for malicious codes to change the indirect jump target such that longjmp > wil > >> jump to the wrong place. Use NOTRACK prefix here defeats the purpose > of > >> indirect branch tracking in CET. > >> > > > > Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may > > return twice, similar to setjmp. > > > > Here is the GCC patch: > > > diff --git a/gcc/calls.c b/gcc/calls.c > index 19c95b8455b..d1f436dfa91 100644 > --- a/gcc/calls.c > +++ b/gcc/calls.c > @@ -604,7 +604,7 @@ special_function_p (const_tree fndecl, int flags) > name_decl = DECL_NAME (cgraph_node::get (fndecl)->orig_decl); > > if (fndecl && name_decl > - && IDENTIFIER_LENGTH (name_decl) <= 11 > + && IDENTIFIER_LENGTH (name_decl) <= 18 > /* Exclude functions not at the file scope, or not `extern', > since they are not the magic functions we would otherwise > think they are. > @@ -637,8 +637,8 @@ special_function_p (const_tree fndecl, int flags) > } > > /* ECF_RETURNS_TWICE is safe even for -ffreestanding. */ > - if (! strcmp (tname, "setjmp") > - || ! strcmp (tname, "sigsetjmp") > + if (! strncmp (tname, "setjmp", 6) > + || ! strncmp (tname, "sigsetjmp", 9) > || ! strcmp (name, "savectx") > || ! strcmp (name, "vfork") > || ! strcmp (name, "getcontext")) Should it be compared with the whole function name (__setjmp_cancel and __sigsetjmp_cancel) as something like setjmp_my_func will be also detected? Also there was an error in detection of __getcontext, which is 12 bytes, but it will be fixed by this patch. Igor > -- > H.J.
On Thu, Mar 8, 2018 at 4:24 AM, Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> wrote: >> -----Original Message----- >> From: H.J. Lu [mailto:hjl.tools@gmail.com] >> Sent: Wednesday, March 7, 2018 11:07 PM >> To: Carlos O'Donell <carlos@redhat.com>; Tsimbalist, Igor V >> <igor.v.tsimbalist@intel.com> >> Cc: Florian Weimer <fw@deneb.enyo.de>; GNU C Library <libc- >> alpha@sourceware.org> >> Subject: Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf >> >> On Wed, Mar 7, 2018 at 12:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> > On Wed, Mar 7, 2018 at 11:47 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> >> On Wed, Mar 7, 2018 at 9:34 AM, Carlos O'Donell <carlos@redhat.com> >> wrote: >> >>> On 03/07/2018 03:56 AM, H.J. Lu wrote: >> >>>>>> 1. I have to add __setjmp_cancel and __sigsetjmp_cancel which >> won't >> >>>>>> save and restore shadow stack register. >> >>>> >> >>>> I have been testing this. I ran into one issue. GCC knows that setjmp >> will >> >>>> return via longjmp and inserts ENDBR after it. But it doesn't know >> >>>> __setjmp_cancel and __sigsetjmp_cancel. We can either add them to >> GCC >> >>>> or add NOTRACK prefix to the corresponding longjmps. >> >>> >> >>> I would rather GCC did not know about these implementation details. >> >>> >> >>> I have no objection to the NOTRACK prefix in the corresponding >> longjmps. >> >>> >> >>> What would be a downside to this choice? >> >>> >> >> >> >> NOTRACK prefix is typically generated by compiler for switch table. >> Compiler >> >> knows each indirect jump target is valid and pointer load for indirect >> jump is >> >> generated by compiler in read-only section. This is pretty safe since there >> is >> >> very little chance for malicious codes to temper the pointer value. >> However, >> >> in case of longjmp, the indirect jump target is in jmpbuf. There is >> >> a possilibty >> >> for malicious codes to change the indirect jump target such that longjmp >> wil >> >> jump to the wrong place. Use NOTRACK prefix here defeats the purpose >> of >> >> indirect branch tracking in CET. >> >> >> > >> > Also GCC needs to know that __setjmp_cancel and __sigsetjmp_cancel may >> > return twice, similar to setjmp. >> > >> >> Here is the GCC patch: >> >> >> diff --git a/gcc/calls.c b/gcc/calls.c >> index 19c95b8455b..d1f436dfa91 100644 >> --- a/gcc/calls.c >> +++ b/gcc/calls.c >> @@ -604,7 +604,7 @@ special_function_p (const_tree fndecl, int flags) >> name_decl = DECL_NAME (cgraph_node::get (fndecl)->orig_decl); >> >> if (fndecl && name_decl >> - && IDENTIFIER_LENGTH (name_decl) <= 11 >> + && IDENTIFIER_LENGTH (name_decl) <= 18 >> /* Exclude functions not at the file scope, or not `extern', >> since they are not the magic functions we would otherwise >> think they are. >> @@ -637,8 +637,8 @@ special_function_p (const_tree fndecl, int flags) >> } >> >> /* ECF_RETURNS_TWICE is safe even for -ffreestanding. */ >> - if (! strcmp (tname, "setjmp") >> - || ! strcmp (tname, "sigsetjmp") >> + if (! strncmp (tname, "setjmp", 6) >> + || ! strncmp (tname, "sigsetjmp", 9) >> || ! strcmp (name, "savectx") >> || ! strcmp (name, "vfork") >> || ! strcmp (name, "getcontext")) > > Should it be compared with the whole function name (__setjmp_cancel and > __sigsetjmp_cancel) as something like setjmp_my_func will be also detected? True. This is the patch I have tested: https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a > Also there was an error in detection of __getcontext, which is 12 bytes, but it > will be fixed by this patch.
On 03/08/2018 04:48 AM, H.J. Lu wrote: > True. This is the patch I have tested: > > https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a I assume NOTRACK prefix is what Intel calls 'NO_TRACK_EN' in the CET documentation since binutils uses the 0x3E prefix for it and that matches the Intel CET docs. Please correct me if I'm wrong. In which case I agree, using NOTRACK is going to prevent a useful use of CET against writes to the cancellation jump buffer. This patch looks good to me, but is not a *correctness* issue, it is simply that we want to extend coverage to the private cancellation setjmp/longjmp buffers. Is there any way we can do this with source markup instead of via a fragile list in the compiler? Presumably users may want to markup their own code like this also if they have custom implementations of functions that behave like setjmp/longjmp?
On Thu, Mar 8, 2018 at 4:47 PM, Carlos O'Donell <carlos@redhat.com> wrote: > On 03/08/2018 04:48 AM, H.J. Lu wrote: >> True. This is the patch I have tested: >> >> https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a > > I assume NOTRACK prefix is what Intel calls 'NO_TRACK_EN' in the CET > documentation since binutils uses the 0x3E prefix for it and that matches > the Intel CET docs. Please correct me if I'm wrong. That is correct. > In which case I agree, using NOTRACK is going to prevent a useful use > of CET against writes to the cancellation jump buffer. True. > This patch looks good to me, but is not a *correctness* issue, it is > simply that we want to extend coverage to the private cancellation > setjmp/longjmp buffers. > > Is there any way we can do this with source markup instead of via > a fragile list in the compiler? > > Presumably users may want to markup their own code like this also > if they have custom implementations of functions that behave like > setjmp/longjmp? > Yes, we can use __attribute__((__returns_twice__)). I updated hjl/setjmp/cancel branch to do that. No GCC changes are needed.
On 03/08/2018 10:23 PM, H.J. Lu wrote: > On Thu, Mar 8, 2018 at 4:47 PM, Carlos O'Donell <carlos@redhat.com> wrote: >> On 03/08/2018 04:48 AM, H.J. Lu wrote: >>> True. This is the patch I have tested: >>> >>> https://github.com/hjl-tools/gcc/commit/e98087865405f051e93d5f35588789ef9686db4a >> >> I assume NOTRACK prefix is what Intel calls 'NO_TRACK_EN' in the CET >> documentation since binutils uses the 0x3E prefix for it and that matches >> the Intel CET docs. Please correct me if I'm wrong. > > That is correct. > >> In which case I agree, using NOTRACK is going to prevent a useful use >> of CET against writes to the cancellation jump buffer. > > True. > >> This patch looks good to me, but is not a *correctness* issue, it is >> simply that we want to extend coverage to the private cancellation >> setjmp/longjmp buffers. >> >> Is there any way we can do this with source markup instead of via >> a fragile list in the compiler? >> >> Presumably users may want to markup their own code like this also >> if they have custom implementations of functions that behave like >> setjmp/longjmp? >> > > Yes, we can use __attribute__((__returns_twice__)). I updated > hjl/setjmp/cancel branch to do that. No GCC changes are needed. Is the next step for me to do another round of review on this branch? Cheers, Carlos.
diff --git a/gcc/calls.c b/gcc/calls.c index 19c95b8455b..d1f436dfa91 100644 --- a/gcc/calls.c +++ b/gcc/calls.c @@ -604,7 +604,7 @@ special_function_p (const_tree fndecl, int flags) name_decl = DECL_NAME (cgraph_node::get (fndecl)->orig_decl); if (fndecl && name_decl - && IDENTIFIER_LENGTH (name_decl) <= 11 + && IDENTIFIER_LENGTH (name_decl) <= 18 /* Exclude functions not at the file scope, or not `extern', since they are not the magic functions we would otherwise think they are. @@ -637,8 +637,8 @@ special_function_p (const_tree fndecl, int flags) } /* ECF_RETURNS_TWICE is safe even for -ffreestanding. */ - if (! strcmp (tname, "setjmp") - || ! strcmp (tname, "sigsetjmp") + if (! strncmp (tname, "setjmp", 6) + || ! strncmp (tname, "sigsetjmp", 9) || ! strcmp (name, "savectx") || ! strcmp (name, "vfork") || ! strcmp (name, "getcontext"))