| Message ID | 20150331094702.GC52842@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On 03/31/2015 03:47 AM, Ilya Enkovich wrote: > On 23 Mar 13:19, Ilya Enkovich wrote: >> Hi, >> >> May this patch go into trunk at this point? It is very important for >> dynamic MPX codes. >> >> Thanks, >> Ilya >> > > I additionally documented changes in invoke.texi. OK for trunk? > > Thanks, > Ilya > -- > gcc/ > > 2015-03-31 Ilya Enkovich <ilya.enkovich@intel.com> > > PR driver/65444 > * config/i386/linux-common.h (MPX_SPEC): New. > (CHKP_SPEC): Add MPX_SPEC. > * doc/invoke.texi (-fcheck-pointer-boudns): Document > possible issues with '-z bndplt' support in linker. > > libmpx/ > > 2015-03-31 Ilya Enkovich <ilya.enkovich@intel.com> > > PR driver/65444 > * configure.ac: Add check for '-z bndplt' support > by linker. Add link_mpx output variable. > * libmpx.spec.in (link_mpx): New. > * configure: Regenerate. Just to make sure I understand. With this patch we conditionally pass -z bndplt based on whether or not it's supported by the linker we find when we configure GCC. Failure to pass -z bndplt won't cause the program to misbehave, but will limit the effectiveness of MPX. Gold doesn't support -z bndplt, just newer versions of the BFD linker. Gold issues an error for -z bndplt, old BFD linkers will issue a warning. There are, at least in theory, use cases where we might not have a PLT and thus -z bndplt wouldn't make sense anyway. I guess the one thing I don't like here is that whether or not to pass -z bndplt is made at the time we configure GCC. Yet, it's trivial for someone to change the system linker later, either to gold or from an old BFD linker that doesn't support -z bndplt to one that does support -z bndplt. [ Note we have the same issue with certain assembler feature tests. ] I'm not aware of any real infrastructure in GCC to query the behavior of the linker at link time and then customize the options passed. So if it's going to be configurable, then that's the only time to do the test. I strongly disagree with HJ's assertion that we should always pass the flag, regardless of the underlying linker. So, in an ideal world, we'd query the linker at link time and pass the flag anytime we have a linker that supports the capability and perhaps warn if the linker doesn't support that capability. Given we're not in that ideal world, I think Ilya's patch is reasonable and should be installed. jeff
On Wed, Apr 1, 2015 at 9:34 PM, Jeff Law <law@redhat.com> wrote: > On 03/31/2015 03:47 AM, Ilya Enkovich wrote: >> >> On 23 Mar 13:19, Ilya Enkovich wrote: >>> >>> Hi, >>> >>> May this patch go into trunk at this point? It is very important for >>> dynamic MPX codes. >>> >>> Thanks, >>> Ilya >>> >> >> I additionally documented changes in invoke.texi. OK for trunk? >> >> Thanks, >> Ilya >> -- >> gcc/ >> >> 2015-03-31 Ilya Enkovich <ilya.enkovich@intel.com> >> >> PR driver/65444 >> * config/i386/linux-common.h (MPX_SPEC): New. >> (CHKP_SPEC): Add MPX_SPEC. >> * doc/invoke.texi (-fcheck-pointer-boudns): Document >> possible issues with '-z bndplt' support in linker. >> >> libmpx/ >> >> 2015-03-31 Ilya Enkovich <ilya.enkovich@intel.com> >> >> PR driver/65444 >> * configure.ac: Add check for '-z bndplt' support >> by linker. Add link_mpx output variable. >> * libmpx.spec.in (link_mpx): New. >> * configure: Regenerate. > > Just to make sure I understand. With this patch we conditionally pass -z > bndplt based on whether or not it's supported by the linker we find when we > configure GCC. > > Failure to pass -z bndplt won't cause the program to misbehave, but will > limit the effectiveness of MPX. > > Gold doesn't support -z bndplt, just newer versions of the BFD linker. > > Gold issues an error for -z bndplt, old BFD linkers will issue a warning. > > There are, at least in theory, use cases where we might not have a PLT and > thus -z bndplt wouldn't make sense anyway. > > I guess the one thing I don't like here is that whether or not to pass -z > bndplt is made at the time we configure GCC. Yet, it's trivial for someone > to change the system linker later, either to gold or from an old BFD linker > that doesn't support -z bndplt to one that does support -z bndplt. > > [ Note we have the same issue with certain assembler feature tests. ] > > I'm not aware of any real infrastructure in GCC to query the behavior of the > linker at link time and then customize the options passed. So if it's going > to be configurable, then that's the only time to do the test. > > I strongly disagree with HJ's assertion that we should always pass the flag, > regardless of the underlying linker. > > So, in an ideal world, we'd query the linker at link time and pass the flag > anytime we have a linker that supports the capability and perhaps warn if > the linker doesn't support that capability. > > Given we're not in that ideal world, I think Ilya's patch is reasonable and > should be installed. Without proper PLT for MPX, all external function calls will clear bound registers. MPX is a security feature. Cyber criminals only need to get it right 1 time. An organization who uses MPX for cyber security may not realize they leave a door open due to an old linker. What I want to avoid is 2 years from now, bank of foobar comes out saying that they thought they were protected by MPX, but somehow MPX didn't catch a buffer overflow it was supposed to and there was no compiler error message to warn programmers.
On Tue, 31 Mar 2015, Ilya Enkovich wrote: > +library. It also passes '-z bndplt' to a linker in case it supports this > +option (which is checked on libmpx configuration). Note that old versions > +of linker may ignore option. Gold linker doesn't support '-z bndplt' > +option. With no '-z bndplt' support in linker all calls to dynamic libraries > +lose passed bounds reducing overall protection level. It's highly > +recommended to use linker with '-z bndplt' support. In case such linker > +is not available it is adviced to always use @option{-static-libmpxwrappers} > +for better protection level or use @option{-static} to completely avoid > +external calls to dynamic libraries. MPX-based instrumentation Use @samp{-z bndplt} rather than '' quoting (but Sandra may have further advice on the substance of this documentation).
On 04/03/2015 01:34 PM, Joseph Myers wrote: > On Tue, 31 Mar 2015, Ilya Enkovich wrote: > >> +library. It also passes '-z bndplt' to a linker in case it supports this >> +option (which is checked on libmpx configuration). Note that old versions >> +of linker may ignore option. Gold linker doesn't support '-z bndplt' >> +option. With no '-z bndplt' support in linker all calls to dynamic libraries >> +lose passed bounds reducing overall protection level. It's highly >> +recommended to use linker with '-z bndplt' support. In case such linker >> +is not available it is adviced to always use @option{-static-libmpxwrappers} >> +for better protection level or use @option{-static} to completely avoid >> +external calls to dynamic libraries. MPX-based instrumentation > > Use @samp{-z bndplt} rather than '' quoting (but Sandra may have further > advice on the substance of this documentation). To tell the truth, I can't figure out what this means from a user perspective. How does a user know whether the linker option is being ignored, or if they have a new enough linker? If the linker available at configuration time doesn't support the option, does that mean the option will never be passed and users will never know that there are gaping holes in the pointer bounds checking? My suggestion would be to pass the option unconditionally and make the documentation say something like It also passes @option{-z bndplt} to the linker. LD version xxx or later is required to use this feature. If no linker support for @option{-z bndplt} is available, you should link with @option{-static-libmpxwrappers} or @option{-static} instead; otherwise calls to dynamic libraries lose bounds checking protection. ... where you need to fill in "version xxx" appropriately. -Sandra
On Sun, Apr 5, 2015 at 6:44 PM, Sandra Loosemore <sandra@codesourcery.com> wrote: > On 04/03/2015 01:34 PM, Joseph Myers wrote: >> >> On Tue, 31 Mar 2015, Ilya Enkovich wrote: >> >>> +library. It also passes '-z bndplt' to a linker in case it supports >>> this >>> +option (which is checked on libmpx configuration). Note that old >>> versions >>> +of linker may ignore option. Gold linker doesn't support '-z bndplt' >>> +option. With no '-z bndplt' support in linker all calls to dynamic >>> libraries >>> +lose passed bounds reducing overall protection level. It's highly >>> +recommended to use linker with '-z bndplt' support. In case such linker >>> +is not available it is adviced to always use >>> @option{-static-libmpxwrappers} >>> +for better protection level or use @option{-static} to completely avoid >>> +external calls to dynamic libraries. MPX-based instrumentation >> >> >> Use @samp{-z bndplt} rather than '' quoting (but Sandra may have further >> advice on the substance of this documentation). > > > To tell the truth, I can't figure out what this means from a user > perspective. How does a user know whether the linker option is being > ignored, or if they have a new enough linker? If the linker available at > configuration time doesn't support the option, does that mean the option > will never be passed and users will never know that there are gaping holes > in the pointer bounds checking? > > My suggestion would be to pass the option unconditionally and make the > documentation say something like I totally agree with it. > It also passes @option{-z bndplt} to the linker. LD version xxx or later is > required to use this feature. If no linker support for @option{-z bndplt} > is available, you should link with @option{-static-libmpxwrappers} or > @option{-static} instead; otherwise calls to dynamic libraries lose bounds > checking protection. > This implies that -static-libmpxwrappers will cover all dynamic libraries, which isn't true. -static-libmpxwrappers only covers calls to functions defined in libmpxwrappers.so and leaves calls to functions defined in other dynamic libraries open to buffer overflow.
On 04/05/2015 08:35 PM, H.J. Lu wrote: > On Sun, Apr 5, 2015 at 6:44 PM, Sandra Loosemore > <sandra@codesourcery.com> wrote: >> [snip snip] >> >> To tell the truth, I can't figure out what this means from a user >> perspective. How does a user know whether the linker option is being >> ignored, or if they have a new enough linker? If the linker available at >> configuration time doesn't support the option, does that mean the option >> will never be passed and users will never know that there are gaping holes >> in the pointer bounds checking? >> >> My suggestion would be to pass the option unconditionally and make the >> documentation say something like > > I totally agree with it. > >> It also passes @option{-z bndplt} to the linker. LD version xxx or later is >> required to use this feature. If no linker support for @option{-z bndplt} >> is available, you should link with @option{-static-libmpxwrappers} or >> @option{-static} instead; otherwise calls to dynamic libraries lose bounds >> checking protection. >> > > This implies that -static-libmpxwrappers will cover all dynamic libraries, > which isn't true. -static-libmpxwrappers only covers calls to functions > defined in libmpxwrappers.so and leaves calls to functions defined in > other dynamic libraries open to buffer overflow. See, I said I didn't really understand the details. ;-) Just strike the reference to -static-libmpxwrappers entirely, then. -Sandra
diff --git a/gcc/config/i386/linux-common.h b/gcc/config/i386/linux-common.h index 9c6560b..dd79ec6 100644 --- a/gcc/config/i386/linux-common.h +++ b/gcc/config/i386/linux-common.h @@ -59,6 +59,11 @@ along with GCC; see the file COPYING3. If not see %:include(libmpx.spec)%(link_libmpx)" #endif +#ifndef MPX_SPEC +#define MPX_SPEC "\ + %{mmpx:%{fcheck-pointer-bounds:%{!static:%:include(libmpx.spec)%(link_mpx)}}}" +#endif + #ifndef LIBMPX_SPEC #if defined(HAVE_LD_STATIC_DYNAMIC) #define LIBMPX_SPEC "\ @@ -89,5 +94,5 @@ along with GCC; see the file COPYING3. If not see #ifndef CHKP_SPEC #define CHKP_SPEC "\ -%{!nostdlib:%{!nodefaultlibs:" LIBMPX_SPEC LIBMPXWRAPPERS_SPEC "}}" +%{!nostdlib:%{!nodefaultlibs:" LIBMPX_SPEC LIBMPXWRAPPERS_SPEC "}}" MPX_SPEC #endif diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index bf8afad..c058710 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -5857,7 +5857,16 @@ MPX-based instrumentation requires a runtime library to enable MPX in hardware and handle bounds violation signals. By default when @option{-fcheck-pointer-bounds} and @option{-mmpx} options are used to link a program, the GCC driver -links against the @file{libmpx} runtime library. MPX-based instrumentation +links against the @file{libmpx} runtime library and @file{libmpxwrappers} +library. It also passes '-z bndplt' to a linker in case it supports this +option (which is checked on libmpx configuration). Note that old versions +of linker may ignore option. Gold linker doesn't support '-z bndplt' +option. With no '-z bndplt' support in linker all calls to dynamic libraries +lose passed bounds reducing overall protection level. It's highly +recommended to use linker with '-z bndplt' support. In case such linker +is not available it is adviced to always use @option{-static-libmpxwrappers} +for better protection level or use @option{-static} to completely avoid +external calls to dynamic libraries. MPX-based instrumentation may be used for debugging and also may be included in production code to increase program security. Depending on usage, you may have different requirements for the runtime library. The current version diff --git a/libmpx/configure.ac b/libmpx/configure.ac index fe0d3f2..3f8b50f 100644 --- a/libmpx/configure.ac +++ b/libmpx/configure.ac @@ -40,7 +40,18 @@ AC_MSG_RESULT($LIBMPX_SUPPORTED) AM_CONDITIONAL(LIBMPX_SUPPORTED, [test "x$LIBMPX_SUPPORTED" = "xyes"]) link_libmpx="-lpthread" +link_mpx="" +AC_MSG_CHECKING([whether ld accepts -z bndplt]) +echo "int main() {};" > conftest.c +if AC_TRY_COMMAND([${CC} ${CFLAGS} -Wl,-z,bndplt -o conftest conftest.c 1>&AS_MESSAGE_LOG_FD]) +then + AC_MSG_RESULT([yes]) + link_mpx="$link_mpx -z bndplt" +else + AC_MSG_RESULT([no]) +fi AC_SUBST(link_libmpx) +AC_SUBST(link_mpx) AM_INIT_AUTOMAKE(foreign no-dist no-dependencies) AM_ENABLE_MULTILIB(, ..) diff --git a/libmpx/libmpx.spec.in b/libmpx/libmpx.spec.in index a265e28..34d0bdf 100644 --- a/libmpx/libmpx.spec.in +++ b/libmpx/libmpx.spec.in @@ -1,3 +1,5 @@ # This spec file is read by gcc when linking. It is used to specify the -# standard libraries we need in order to link with libcilkrts. +# standard libraries we need in order to link with libmpx. *link_libmpx: @link_libmpx@ + +*link_mpx: @link_mpx@