| Message ID | 20230620181910.1506893-2-fberat@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Allow glibc to be built with _FORTIFY_SOURCE | expand |
On Tue, 20 Jun 2023, Frédéric Bérat via Libc-alpha wrote: > It is now possible to enable fortification. > The level may be given as parameter, if none is provided, the configure > script will determine what is the highest level possible that can be set > considering GCC built-ins availability and set it. > If level is explicitly set to 3, configure checks if the compiler > supports the built-in function necessary for it or raise an error if it > isn't. A new configure option should be documented in manual/install.texi with INSTALL regenerated, and should be mentioned in NEWS as well.
On Tue, Jun 20, 2023 at 8:19 PM Frédéric Bérat <fberat@redhat.com> wrote: > > It is now possible to enable fortification. > The level may be given as parameter, if none is provided, the configure > script will determine what is the highest level possible that can be set > considering GCC built-ins availability and set it. > If level is explicitly set to 3, configure checks if the compiler > supports the built-in function necessary for it or raise an error if it > isn't. > > The result of the configure checks is 2 variables, $fortify_source and > $no_fortify_source that are used to appropriately populate CFLAGS. > > Since the feature needs some of the routines provided by Glibc, these > are excluded from the fortification. > --- > Makeconfig | 33 ++++++++++++++++++++++++--- > config.make.in | 3 ++- > configure.ac | 60 +++++++++++++++++++++++++++++++++++--------------- > elf/rtld-Rules | 2 +- > 4 files changed, 75 insertions(+), 23 deletions(-) > > diff --git a/Makeconfig b/Makeconfig > index 2514db35f6..59fbd9ebf9 100644 > --- a/Makeconfig > +++ b/Makeconfig > @@ -543,12 +543,13 @@ endif # +link > # ARM, gcc always produces different debugging symbols when invoked with > # a -O greater than 0 than when invoked with -O0, regardless of anything else > # we're using to suppress optimizations. Therefore, we need to explicitly pass > -# -O0 to it through CFLAGS. > +# -O0 to it through CFLAGS. By side effect, any fortification needs to be > +# disabled as it needs -O greater than 0. > # Additionally, the build system will try to -include $(common-objpfx)/config.h > # when compiling the tests, which will throw an error if some special macros > # (such as __OPTIMIZE__ and IS_IN_build) aren't defined. To avoid this, we > # tell gcc to define IS_IN_build. > -CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build > +CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build $(no-fortify-source) > > ifeq (yes,$(build-shared)) > # These indicate whether to link using the built ld.so or the installed one. > @@ -901,6 +902,16 @@ define elide-stack-protector > $(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-stack-protector)) > endef > > +# We might want to compile with fortify-source > +ifneq ($(fortify-source),) > ++fortify-source=$(fortify-source) > +endif > + > +# Some routine can't be fortified like the ones used by fortify > +define elide-fortify-source > +$(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-fortify-source)) > +endef > + > # The program that makes Emacs-style TAGS files. > ETAGS := etags > > @@ -961,6 +972,16 @@ endif # $(+cflags) == "" > $(+stack-protector) -fno-common > +gcc-nowarn := -w > > +# We must filter out elf because the early bootstrap of the dynamic loader > +# cannot be fortified. Likewise we exclude dlfcn because it is entangled > +# with the loader. We must filter out csu because early startup, like the > +# loader, cannot be fortified. Lastly debug is the fortification routines > +# themselves and they cannot be fortified. > +do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) > +ifeq ($(do-fortify),$(subdir)) > ++cflags += $(+fortify-source) > +endif This needs to be adapted to deal with compilers that define "_FORTIFY_SOURCE" by default (checked on ubuntu 22.04): +do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) +ifeq ($(do-fortify),$(subdir)) ++cflags += $(+fortify-source) +else ++cflags += $(no-fortify-source) +endif That way, we ensure that even if _FORTIFY_SOURCE is enabled at system level, we don't build these subdirectories with it. > + > # Each sysdeps directory can contain header files that both will be > # used to compile and will be installed. Each can also contain an > # include/ subdirectory, whose header files will be used to compile > @@ -1010,7 +1031,7 @@ module-cppflags = $(if $(filter %.mk.i %.v.i,$(@F)),,$(module-cppflags-real)) > # Note that we can't use -std=* in CPPFLAGS, because it overrides > # the implicit -lang-asm and breaks cpp behavior for .S files--notably > # it causes cpp to stop predefining __ASSEMBLER__. > -CPPFLAGS = $(config-extra-cppflags) $(CPPUNDEFS) $(CPPFLAGS-config) \ > +CPPFLAGS = $(config-extra-cppflags) $(CPPFLAGS-config) \ > $($(subdir)-CPPFLAGS) \ > $(+includes) $(defines) $(module-cppflags) \ > -include $(..)include/libc-symbols.h $(sysdep-CPPFLAGS) \ > @@ -1049,6 +1070,8 @@ object-suffixes := > CPPFLAGS-.o = $(pic-default) > # libc.a must be compiled with -fPIE/-fpie for static PIE. > CFLAGS-.o = $(filter %frame-pointer,$(+cflags)) $(pie-default) > +CFLAGS-.o += $(call elide-fortify-source,.o,$(routines_no_fortify)) > +CFLAGS-.o += $(call elide-fortify-source,_chk.o,$(routines_no_fortify)) > libtype.o := lib%.a > object-suffixes += .o > ifeq (yes,$(build-shared)) > @@ -1058,6 +1081,8 @@ object-suffixes += .os > pic-cppflags = -DPIC -DSHARED > CPPFLAGS-.os = $(pic-cppflags) > CFLAGS-.os = $(filter %frame-pointer,$(+cflags)) $(pic-ccflag) > +CFLAGS-.os += $(call elide-fortify-source,.os,$(routines_no_fortify)) > +CFLAGS-.os += $(call elide-fortify-source,_chk.os,$(routines_no_fortify)) > libtype.os := lib%_pic.a > # This can be changed by a sysdep makefile > pic-ccflag = -fPIC > @@ -1077,6 +1102,8 @@ object-suffixes += .op > CPPFLAGS-.op = -DPROF $(pic-default) > # libc_p.a must be compiled with -fPIE/-fpie for static PIE. > CFLAGS-.op = -pg $(pie-default) > +CFLAGS-.op += $(call elide-fortify-source,.op,$(routines_no_fortify)) > +CFLAGS-.op += $(call elide-fortify-source,_chk.op,$(routines_no_fortify)) > libtype.op = lib%_p.a > endif > > diff --git a/config.make.in b/config.make.in > index 4afd37feaf..d487a4f4e9 100644 > --- a/config.make.in > +++ b/config.make.in > @@ -64,6 +64,8 @@ have-fpie = @libc_cv_fpie@ > have-ssp = @libc_cv_ssp@ > stack-protector = @stack_protector@ > no-stack-protector = @no_stack_protector@ > +fortify-source = @fortify_source@ > +no-fortify-source = @no_fortify_source@ > have-selinux = @have_selinux@ > have-libaudit = @have_libaudit@ > have-libcap = @have_libcap@ > @@ -101,7 +103,6 @@ CXX = @CXX@ > BUILD_CC = @BUILD_CC@ > CFLAGS = @CFLAGS@ > CPPFLAGS-config = @CPPFLAGS@ > -CPPUNDEFS = @CPPUNDEFS@ > extra-nonshared-cflags = @extra_nonshared_cflags@ > rtld-early-cflags = @rtld_early_cflags@ > ASFLAGS-config = @ASFLAGS_config@ > diff --git a/configure.ac b/configure.ac > index 21879c933c..ec4de6e551 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -466,6 +466,17 @@ AC_ARG_ENABLE([scv], > > AS_IF([[test "$use_scv" != "no"]],[AC_DEFINE(USE_PPC_SCV)]) > > +dnl Build glibc with _FORTIFY_SOURCE > +AC_ARG_ENABLE(fortify-source, > + AS_HELP_STRING([--enable-fortify-source@<:@=1|2|3@:>@], > + [Use -D_FORTIFY_SOURCE=[1|2|3] to control code hardening, defaults to highest possible value for your system]), > + [enable_fortify_source=$enableval], > + [enable_fortify_source=no]) > +case "$enable_fortify_source" in > +1|2|3|no|yes) ;; > +*) AC_MSG_ERROR([Not a valid argument for --enable-fortify-source: "$enable_fortify_source"]);; > +esac > + > # We keep the original values in `$config_*' and never modify them, so we > # can write them unchanged into config.make. Everything else uses > # $machine, $vendor, and $os, and changes them whenever convenient. > @@ -1559,24 +1570,37 @@ if test "x$have_selinux" = xyes; then > fi > AC_SUBST(have_selinux) > > -CPPUNDEFS= > -dnl Check for silly hacked compilers predefining _FORTIFY_SOURCE. > -dnl Since we are building the implementations of the fortified functions here, > -dnl having the macro defined interacts very badly. > -dnl _FORTIFY_SOURCE requires compiler optimization level 1 (gcc -O1) > -dnl and above (see "man FEATURE_TEST_MACROS"). > -dnl So do NOT replace AC_COMPILE_IFELSE with AC_PREPROC_IFELSE. > -AC_CACHE_CHECK([for _FORTIFY_SOURCE predefine], libc_cv_predef_fortify_source, > -[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[]], [[ > -#ifdef _FORTIFY_SOURCE > -# error bogon > -#endif]])], > - [libc_cv_predef_fortify_source=no], > - [libc_cv_predef_fortify_source=yes])]) > -if test $libc_cv_predef_fortify_source = yes; then > - CPPUNDEFS="${CPPUNDEFS:+$CPPUNDEFS }-U_FORTIFY_SOURCE" > -fi > -AC_SUBST(CPPUNDEFS) > +dnl Check if we support the requested _FORTIFY_SOURCE level > +dnl If not, then don't use it. > +dnl Note that _FORTIFY_SOURCE may have been set through FLAGS too. > +dnl _FORTIFY_SOURCE value will be selectively disabled for function that can't > +dnl support it > +fortify_source="" > +no_fortify_source="-Wp,-U_FORTIFY_SOURCE" > + > +AC_CACHE_CHECK([for __builtin_dynamic_object_size], [libc_cv___builtin_dynamic_object_size], [ > + AC_LINK_IFELSE([AC_LANG_PROGRAM([], [__builtin_dynamic_object_size("", 0)])], > + [libc_cv___builtin_dynamic_object_size=yes > + AS_IF([test "$enable_fortify_source" = yes], [enable_fortify_source=3])], > + [libc_cv___builtin_dynamic_object_size=no > + AS_IF([test "$enable_fortify_source" = yes], [enable_fortify_source=2])]) > +]) > + > +AS_CASE([$enable_fortify_source], > + [1|2], [libc_cv_fortify_source=yes], > + [3], [AS_IF([test "$libc_cv___builtin_dynamic_object_size" = yes], > + [libc_cv_fortify_source=yes], > + [AC_MSG_ERROR([Compiler doesn't provide necessary support for _FORTIFY_SOURCE=3])])], > + [libc_cv_fortify_source=no]) > + > +AS_IF([test "$libc_cv_fortify_source" = yes], > + [fortify_source="${no_fortify_source},-D_FORTIFY_SOURCE=${enable_fortify_source}"] > + ) > + > +AC_SUBST(enable_fortify_source) > +AC_SUBST(libc_cv_fortify_source) > +AC_SUBST(no_fortify_source) > +AC_SUBST(fortify_source) > > dnl Starting with binutils 2.35, GAS can attach multiple symbol versions > dnl to one symbol (PR 23840). > diff --git a/elf/rtld-Rules b/elf/rtld-Rules > index 56bc4543de..365a3408f3 100644 > --- a/elf/rtld-Rules > +++ b/elf/rtld-Rules > @@ -144,6 +144,6 @@ cpp-srcs-left := $(rtld-modules:%.os=%) > lib := rtld > include $(patsubst %,$(..)libof-iterator.mk,$(cpp-srcs-left)) > > -rtld-CFLAGS += $(no-stack-protector) > +rtld-CFLAGS += $(no-stack-protector) $(no-fortify-source) > > endif > -- > 2.41.0 >
On Tue, Jun 20, 2023 at 10:42 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Tue, 20 Jun 2023, Frédéric Bérat via Libc-alpha wrote: > > > It is now possible to enable fortification. > > The level may be given as parameter, if none is provided, the configure > > script will determine what is the highest level possible that can be set > > considering GCC built-ins availability and set it. > > If level is explicitly set to 3, configure checks if the compiler > > supports the built-in function necessary for it or raise an error if it > > isn't. > > A new configure option should be documented in manual/install.texi with > INSTALL regenerated, and should be mentioned in NEWS as well. > I'll do it. I'll also squash the configure script refresh in there, and refresh it with autoconf 2.69 as requested in the other patch. > -- > Joseph S. Myers > joseph@codesourcery.com
On 2023-06-21 09:18, Frederic Berat wrote: > On Tue, Jun 20, 2023 at 8:19 PM Frédéric Bérat <fberat@redhat.com> wrote: >> >> It is now possible to enable fortification. >> The level may be given as parameter, if none is provided, the configure >> script will determine what is the highest level possible that can be set >> considering GCC built-ins availability and set it. >> If level is explicitly set to 3, configure checks if the compiler >> supports the built-in function necessary for it or raise an error if it >> isn't. >> >> The result of the configure checks is 2 variables, $fortify_source and >> $no_fortify_source that are used to appropriately populate CFLAGS. >> >> Since the feature needs some of the routines provided by Glibc, these >> are excluded from the fortification. >> --- >> Makeconfig | 33 ++++++++++++++++++++++++--- >> config.make.in | 3 ++- >> configure.ac | 60 +++++++++++++++++++++++++++++++++++--------------- >> elf/rtld-Rules | 2 +- >> 4 files changed, 75 insertions(+), 23 deletions(-) >> >> diff --git a/Makeconfig b/Makeconfig >> index 2514db35f6..59fbd9ebf9 100644 >> --- a/Makeconfig >> +++ b/Makeconfig >> @@ -543,12 +543,13 @@ endif # +link >> # ARM, gcc always produces different debugging symbols when invoked with >> # a -O greater than 0 than when invoked with -O0, regardless of anything else >> # we're using to suppress optimizations. Therefore, we need to explicitly pass >> -# -O0 to it through CFLAGS. >> +# -O0 to it through CFLAGS. By side effect, any fortification needs to be >> +# disabled as it needs -O greater than 0. >> # Additionally, the build system will try to -include $(common-objpfx)/config.h >> # when compiling the tests, which will throw an error if some special macros >> # (such as __OPTIMIZE__ and IS_IN_build) aren't defined. To avoid this, we >> # tell gcc to define IS_IN_build. >> -CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build >> +CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build $(no-fortify-source) >> >> ifeq (yes,$(build-shared)) >> # These indicate whether to link using the built ld.so or the installed one. >> @@ -901,6 +902,16 @@ define elide-stack-protector >> $(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-stack-protector)) >> endef >> >> +# We might want to compile with fortify-source >> +ifneq ($(fortify-source),) >> ++fortify-source=$(fortify-source) >> +endif >> + >> +# Some routine can't be fortified like the ones used by fortify >> +define elide-fortify-source >> +$(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-fortify-source)) >> +endef >> + >> # The program that makes Emacs-style TAGS files. >> ETAGS := etags >> >> @@ -961,6 +972,16 @@ endif # $(+cflags) == "" >> $(+stack-protector) -fno-common >> +gcc-nowarn := -w >> >> +# We must filter out elf because the early bootstrap of the dynamic loader >> +# cannot be fortified. Likewise we exclude dlfcn because it is entangled >> +# with the loader. We must filter out csu because early startup, like the >> +# loader, cannot be fortified. Lastly debug is the fortification routines >> +# themselves and they cannot be fortified. >> +do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) >> +ifeq ($(do-fortify),$(subdir)) >> ++cflags += $(+fortify-source) >> +endif > > This needs to be adapted to deal with compilers that define > "_FORTIFY_SOURCE" by default (checked on ubuntu 22.04): > > +do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) > +ifeq ($(do-fortify),$(subdir)) > ++cflags += $(+fortify-source) > +else > ++cflags += $(no-fortify-source) > +endif > > That way, we ensure that even if _FORTIFY_SOURCE is enabled at system > level, we don't build these subdirectories with it. That's likely true for Gentoo as well. Additionally, those distributions will default to fortification being enabled by default, without --enable-fortify-source. This is probably wrong in terms of compatibility and user expectations, i.e. without the configure flag, the code generated should be like in 2.37, i.e. glibc should strictly not be built with fortification enabled. Sam, do you have an opinion on this? Would it be too surprising for Gentoo packaging/users if glibc 2.38 built with fortification on by default? FWIW, we'll likely flip to enabling fortification by default in 2.39 and make the flag --disable-fortify-source, but that's a different bridge to cross. Thanks, Sid
On Wed, Jun 21, 2023 at 5:26 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: > > On 2023-06-21 09:18, Frederic Berat wrote: > > On Tue, Jun 20, 2023 at 8:19 PM Frédéric Bérat <fberat@redhat.com> wrote: > >> > >> It is now possible to enable fortification. > >> The level may be given as parameter, if none is provided, the configure > >> script will determine what is the highest level possible that can be set > >> considering GCC built-ins availability and set it. > >> If level is explicitly set to 3, configure checks if the compiler > >> supports the built-in function necessary for it or raise an error if it > >> isn't. > >> > >> The result of the configure checks is 2 variables, $fortify_source and > >> $no_fortify_source that are used to appropriately populate CFLAGS. > >> > >> Since the feature needs some of the routines provided by Glibc, these > >> are excluded from the fortification. > >> --- > >> Makeconfig | 33 ++++++++++++++++++++++++--- > >> config.make.in | 3 ++- > >> configure.ac | 60 +++++++++++++++++++++++++++++++++++--------------- > >> elf/rtld-Rules | 2 +- > >> 4 files changed, 75 insertions(+), 23 deletions(-) > >> > >> diff --git a/Makeconfig b/Makeconfig > >> index 2514db35f6..59fbd9ebf9 100644 > >> --- a/Makeconfig > >> +++ b/Makeconfig > >> @@ -543,12 +543,13 @@ endif # +link > >> # ARM, gcc always produces different debugging symbols when invoked with > >> # a -O greater than 0 than when invoked with -O0, regardless of anything else > >> # we're using to suppress optimizations. Therefore, we need to explicitly pass > >> -# -O0 to it through CFLAGS. > >> +# -O0 to it through CFLAGS. By side effect, any fortification needs to be > >> +# disabled as it needs -O greater than 0. > >> # Additionally, the build system will try to -include $(common-objpfx)/config.h > >> # when compiling the tests, which will throw an error if some special macros > >> # (such as __OPTIMIZE__ and IS_IN_build) aren't defined. To avoid this, we > >> # tell gcc to define IS_IN_build. > >> -CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build > >> +CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build $(no-fortify-source) > >> > >> ifeq (yes,$(build-shared)) > >> # These indicate whether to link using the built ld.so or the installed one. > >> @@ -901,6 +902,16 @@ define elide-stack-protector > >> $(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-stack-protector)) > >> endef > >> > >> +# We might want to compile with fortify-source > >> +ifneq ($(fortify-source),) > >> ++fortify-source=$(fortify-source) > >> +endif > >> + > >> +# Some routine can't be fortified like the ones used by fortify > >> +define elide-fortify-source > >> +$(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-fortify-source)) > >> +endef > >> + > >> # The program that makes Emacs-style TAGS files. > >> ETAGS := etags > >> > >> @@ -961,6 +972,16 @@ endif # $(+cflags) == "" > >> $(+stack-protector) -fno-common > >> +gcc-nowarn := -w > >> > >> +# We must filter out elf because the early bootstrap of the dynamic loader > >> +# cannot be fortified. Likewise we exclude dlfcn because it is entangled > >> +# with the loader. We must filter out csu because early startup, like the > >> +# loader, cannot be fortified. Lastly debug is the fortification routines > >> +# themselves and they cannot be fortified. > >> +do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) > >> +ifeq ($(do-fortify),$(subdir)) > >> ++cflags += $(+fortify-source) > >> +endif > > > > This needs to be adapted to deal with compilers that define > > "_FORTIFY_SOURCE" by default (checked on ubuntu 22.04): > > > > +do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) > > +ifeq ($(do-fortify),$(subdir)) > > ++cflags += $(+fortify-source) > > +else > > ++cflags += $(no-fortify-source) > > +endif > > > > That way, we ensure that even if _FORTIFY_SOURCE is enabled at system > > level, we don't build these subdirectories with it. > > That's likely true for Gentoo as well. Additionally, those > distributions will default to fortification being enabled by default, > without --enable-fortify-source. This is probably wrong in terms of > compatibility and user expectations, i.e. without the configure flag, > the code generated should be like in 2.37, i.e. glibc should strictly > not be built with fortification enabled. > > Sam, do you have an opinion on this? Would it be too surprising for > Gentoo packaging/users if glibc 2.38 built with fortification on by > default? FWIW, we'll likely flip to enabling fortification by default > in 2.39 and make the flag --disable-fortify-source, but that's a > different bridge to cross. > Since I'd like to get some more feedback, I'll keep the behavior as-is in v2, i.e.: - If no option is passed to configure, the build environment decides if _FORTIFY_SOURCE should be set (through exported CFLAGS or compiler). In other words the "disable" is not enforced with this patch. - If "--enable-fortify-source" is set, it overrides any system preset. I'd be happy to get more feedback on whether I should enforce disablement or not, especially from people who deal with distribution that set it by default. > Thanks, > Sid >
On 2023-06-22 07:48, Frederic Berat wrote: >> That's likely true for Gentoo as well. Additionally, those >> distributions will default to fortification being enabled by default, >> without --enable-fortify-source. This is probably wrong in terms of >> compatibility and user expectations, i.e. without the configure flag, >> the code generated should be like in 2.37, i.e. glibc should strictly >> not be built with fortification enabled. >> >> Sam, do you have an opinion on this? Would it be too surprising for >> Gentoo packaging/users if glibc 2.38 built with fortification on by >> default? FWIW, we'll likely flip to enabling fortification by default >> in 2.39 and make the flag --disable-fortify-source, but that's a >> different bridge to cross. >> > > Since I'd like to get some more feedback, I'll keep the behavior as-is > in v2, i.e.: > > - If no option is passed to configure, the build environment decides > if _FORTIFY_SOURCE should be set (through exported CFLAGS or > compiler). In other words the "disable" is not enforced with this > patch. > - If "--enable-fortify-source" is set, it overrides any system preset. > > I'd be happy to get more feedback on whether I should enforce > disablement or not, especially from people who deal with distribution > that set it by default. My worry is that with this on Ubuntu/Gentoo, there's no way through configure to disable fortification, which, importantly, is current behaviour. The only way to do it would then be to pass CFLAGS to override the toolchain. It's not the worst problem to have TBH, but I'd like Ubuntu or Gentoo maintainers to explicitly opt into it instead of us forcing it on them. Thanks, Sid
On 22/06/23 08:54, Siddhesh Poyarekar wrote: > On 2023-06-22 07:48, Frederic Berat wrote: >>> That's likely true for Gentoo as well. Additionally, those >>> distributions will default to fortification being enabled by default, >>> without --enable-fortify-source. This is probably wrong in terms of >>> compatibility and user expectations, i.e. without the configure flag, >>> the code generated should be like in 2.37, i.e. glibc should strictly >>> not be built with fortification enabled. >>> >>> Sam, do you have an opinion on this? Would it be too surprising for >>> Gentoo packaging/users if glibc 2.38 built with fortification on by >>> default? FWIW, we'll likely flip to enabling fortification by default >>> in 2.39 and make the flag --disable-fortify-source, but that's a >>> different bridge to cross. >>> >> >> Since I'd like to get some more feedback, I'll keep the behavior as-is >> in v2, i.e.: >> >> - If no option is passed to configure, the build environment decides >> if _FORTIFY_SOURCE should be set (through exported CFLAGS or >> compiler). In other words the "disable" is not enforced with this >> patch. >> - If "--enable-fortify-source" is set, it overrides any system preset. >> >> I'd be happy to get more feedback on whether I should enforce >> disablement or not, especially from people who deal with distribution >> that set it by default. > > My worry is that with this on Ubuntu/Gentoo, there's no way through configure to disable fortification, which, importantly, is current behaviour. The only way to do it would then be to pass CFLAGS to override the toolchain. It's not the worst problem to have TBH, but I'd like Ubuntu or Gentoo maintainers to explicitly opt into it instead of us forcing it on them. Do we really need another configure option? Any extra option requires extra maintainability effort to avoid it to bit-rotten (like --disable-shared [1]), this option would most likely to be always used (specially because distros are enabling fortify as default), and the fortify coverage are a superset of the default prototypes (meaning that programs that built and run with fortify enable should also run without it). Also, this seems to better handled as either through the toolchain default configuration or by CC/CFLAGS. [1] https://sourceware.org/bugzilla/show_bug.cgi?id=20845
On 2023-06-22 08:29, Adhemerval Zanella Netto wrote: >>> I'd be happy to get more feedback on whether I should enforce >>> disablement or not, especially from people who deal with distribution >>> that set it by default. >> >> My worry is that with this on Ubuntu/Gentoo, there's no way through configure to disable fortification, which, importantly, is current behaviour. The only way to do it would then be to pass CFLAGS to override the toolchain. It's not the worst problem to have TBH, but I'd like Ubuntu or Gentoo maintainers to explicitly opt into it instead of us forcing it on them. > > Do we really need another configure option? Any extra option requires extra > maintainability effort to avoid it to bit-rotten (like --disable-shared [1]), > this option would most likely to be always used (specially because distros > are enabling fortify as default), and the fortify coverage are a superset > of the default prototypes (meaning that programs that built and run with > fortify enable should also run without it). The option is needed for the small device users, where they often don't want to build with stack protector, fortification, or really anything that would accidentally make their devices more secure (sorry ;)). The broader plan we had in mind was to disable by default in glibc-2.38 and then enable by default in glibc-2.39. That would also mean adding a b-m-g target with --disable-fortify to ensure that it doesn't bitrot. Alternatively if we're feeling adventurous, we could just enable by default in 2.38 and be done with it. Like you said, most distributions will enable it at first opportunity anyway and doing it by default is easier than modifying the build config to add a flag. Let them add a flag if it's actually breaking things. What do you think? Thanks, Sid
On 22/06/23 09:50, Siddhesh Poyarekar wrote: > On 2023-06-22 08:29, Adhemerval Zanella Netto wrote: >>>> I'd be happy to get more feedback on whether I should enforce >>>> disablement or not, especially from people who deal with distribution >>>> that set it by default. >>> >>> My worry is that with this on Ubuntu/Gentoo, there's no way through configure to disable fortification, which, importantly, is current behaviour. The only way to do it would then be to pass CFLAGS to override the toolchain. It's not the worst problem to have TBH, but I'd like Ubuntu or Gentoo maintainers to explicitly opt into it instead of us forcing it on them. >> >> Do we really need another configure option? Any extra option requires extra >> maintainability effort to avoid it to bit-rotten (like --disable-shared [1]), >> this option would most likely to be always used (specially because distros >> are enabling fortify as default), and the fortify coverage are a superset >> of the default prototypes (meaning that programs that built and run with >> fortify enable should also run without it). > > The option is needed for the small device users, where they often don't want to build with stack protector, fortification, or really anything that would accidentally make their devices more secure (sorry ;)). But which is the size and performance implication for glibc itself by enabling fortify as default? > > The broader plan we had in mind was to disable by default in glibc-2.38 and then enable by default in glibc-2.39. That would also mean adding a b-m-g target with --disable-fortify to ensure that it doesn't bitrot. This has the drawback to require even more computer power to check all possible variants. We already have --disable-default-pie, 3 modes for --enable-stack-protector; and now for fortify it means another option. Each configuration flag adds quadratic more requirement to check if all permutation does not break, which is far from ideal. > > Alternatively if we're feeling adventurous, we could just enable by default in 2.38 and be done with it. Like you said, most distributions will enable it at first opportunity anyway and doing it by default is easier than modifying the build config to add a flag. Let them add a flag if it's actually breaking things. What do you think? > My take is do not add any more configuration flag and if user want to enable fortify the best option would be through CC/CFLAGS. We already have specific Makefile rules to disable where it is not applicable (similar to stack protector), so I think we should follow the same pattern and assume it might be enabled, disable where it is not applicable, and fix the potential issues (as the rest of this patch). In fact, I think we should do something similar to PIE and stack protector.
On 2023-06-22 15:39, Adhemerval Zanella Netto wrote: >> The option is needed for the small device users, where they often don't want to build with stack protector, fortification, or really anything that would accidentally make their devices more secure (sorry ;)). > > But which is the size and performance implication for glibc itself by > enabling fortify as default? IMO it's negligible, but I don't have a stake in that use case. Those who do, tend to worry about a few bytes here and there. >> >> The broader plan we had in mind was to disable by default in glibc-2.38 and then enable by default in glibc-2.39. That would also mean adding a b-m-g target with --disable-fortify to ensure that it doesn't bitrot. > > This has the drawback to require even more computer power to check all > possible variants. We already have --disable-default-pie, 3 modes > for --enable-stack-protector; and now for fortify it means another > option. Each configuration flag adds quadratic more requirement to > check if all permutation does not break, which is far from ideal. ... and on the topic of stack-protector, I wonder if we should make stack-protector=strong the default. > My take is do not add any more configuration flag and if user want to enable > fortify the best option would be through CC/CFLAGS. We already have specific > Makefile rules to disable where it is not applicable (similar to stack > protector), so I think we should follow the same pattern and assume it might > be enabled, disable where it is not applicable, and fix the potential issues > (as the rest of this patch). But isn't that like sweeping the problem under the rug? We'd still have to test the different configurations (or let them rot), just that we'd be testing different CFLAGS instead of configure flags. Sid
Siddhesh Poyarekar <siddhesh@gotplt.org> writes: > On 2023-06-21 09:18, Frederic Berat wrote: >> On Tue, Jun 20, 2023 at 8:19 PM Frédéric Bérat <fberat@redhat.com> wrote: >>> >>> It is now possible to enable fortification. >>> The level may be given as parameter, if none is provided, the configure >>> script will determine what is the highest level possible that can be set >>> considering GCC built-ins availability and set it. >>> If level is explicitly set to 3, configure checks if the compiler >>> supports the built-in function necessary for it or raise an error if it >>> isn't. >>> >>> The result of the configure checks is 2 variables, $fortify_source and >>> $no_fortify_source that are used to appropriately populate CFLAGS. >>> >>> Since the feature needs some of the routines provided by Glibc, these >>> are excluded from the fortification. >>> --- >>> Makeconfig | 33 ++++++++++++++++++++++++--- >>> config.make.in | 3 ++- >>> configure.ac | 60 +++++++++++++++++++++++++++++++++++--------------- >>> elf/rtld-Rules | 2 +- >>> 4 files changed, 75 insertions(+), 23 deletions(-) >>> >>> diff --git a/Makeconfig b/Makeconfig >>> index 2514db35f6..59fbd9ebf9 100644 >>> --- a/Makeconfig >>> +++ b/Makeconfig >>> @@ -543,12 +543,13 @@ endif # +link >>> # ARM, gcc always produces different debugging symbols when invoked with >>> # a -O greater than 0 than when invoked with -O0, regardless of anything else >>> # we're using to suppress optimizations. Therefore, we need to explicitly pass >>> -# -O0 to it through CFLAGS. >>> +# -O0 to it through CFLAGS. By side effect, any fortification needs to be >>> +# disabled as it needs -O greater than 0. >>> # Additionally, the build system will try to -include $(common-objpfx)/config.h >>> # when compiling the tests, which will throw an error if some special macros >>> # (such as __OPTIMIZE__ and IS_IN_build) aren't defined. To avoid this, we >>> # tell gcc to define IS_IN_build. >>> -CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build >>> +CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build $(no-fortify-source) >>> >>> ifeq (yes,$(build-shared)) >>> # These indicate whether to link using the built ld.so or the installed one. >>> @@ -901,6 +902,16 @@ define elide-stack-protector >>> $(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-stack-protector)) >>> endef >>> >>> +# We might want to compile with fortify-source >>> +ifneq ($(fortify-source),) >>> ++fortify-source=$(fortify-source) >>> +endif >>> + >>> +# Some routine can't be fortified like the ones used by fortify >>> +define elide-fortify-source >>> +$(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-fortify-source)) >>> +endef >>> + >>> # The program that makes Emacs-style TAGS files. >>> ETAGS := etags >>> >>> @@ -961,6 +972,16 @@ endif # $(+cflags) == "" >>> $(+stack-protector) -fno-common >>> +gcc-nowarn := -w >>> >>> +# We must filter out elf because the early bootstrap of the dynamic loader >>> +# cannot be fortified. Likewise we exclude dlfcn because it is entangled >>> +# with the loader. We must filter out csu because early startup, like the >>> +# loader, cannot be fortified. Lastly debug is the fortification routines >>> +# themselves and they cannot be fortified. >>> +do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) >>> +ifeq ($(do-fortify),$(subdir)) >>> ++cflags += $(+fortify-source) >>> +endif >> This needs to be adapted to deal with compilers that define >> "_FORTIFY_SOURCE" by default (checked on ubuntu 22.04): >> +do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) >> +ifeq ($(do-fortify),$(subdir)) >> ++cflags += $(+fortify-source) >> +else >> ++cflags += $(no-fortify-source) >> +endif >> That way, we ensure that even if _FORTIFY_SOURCE is enabled at >> system >> level, we don't build these subdirectories with it. > > That's likely true for Gentoo as well. Additionally, those > distributions will default to fortification being enabled by default, > without --enable-fortify-source. This is probably wrong in terms of > compatibility and user expectations, i.e. without the configure flag, > the code generated should be like in 2.37, i.e. glibc should strictly > not be built with fortification enabled. > > Sam, do you have an opinion on this? Would it be too surprising for > Gentoo packaging/users if glibc 2.38 built with fortification on by > default? FWIW, we'll likely flip to enabling fortification by default > in 2.39 and make the flag --disable-fortify-source, but that's a > different bridge to cross. (Sorry for the delay - this got buried. I know it's been merged since.) Thanks for asking! I don't see a problem with this, as we build with F_S=2 even for vanilla users, although I'll have to think about whether or not to keep it at 2 or roll with the glibc default of F_S=3 if the toolchain supports it? best, sam
On Thu, Jun 22, 2023 at 11:26 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: > > On 2023-06-22 15:39, Adhemerval Zanella Netto wrote: > >> The option is needed for the small device users, where they often don't want to build with stack protector, fortification, or really anything that would accidentally make their devices more secure (sorry ;)). > > > > But which is the size and performance implication for glibc itself by > > enabling fortify as default? > > IMO it's negligible, but I don't have a stake in that use case. Those > who do, tend to worry about a few bytes here and there. > > >> > >> The broader plan we had in mind was to disable by default in glibc-2.38 and then enable by default in glibc-2.39. That would also mean adding a b-m-g target with --disable-fortify to ensure that it doesn't bitrot. > > > > This has the drawback to require even more computer power to check all > > possible variants. We already have --disable-default-pie, 3 modes > > for --enable-stack-protector; and now for fortify it means another > > option. Each configuration flag adds quadratic more requirement to > > check if all permutation does not break, which is far from ideal. > > ... and on the topic of stack-protector, I wonder if we should make > stack-protector=strong the default. > > > My take is do not add any more configuration flag and if user want to enable > > fortify the best option would be through CC/CFLAGS. We already have specific > > Makefile rules to disable where it is not applicable (similar to stack > > protector), so I think we should follow the same pattern and assume it might > > be enabled, disable where it is not applicable, and fix the potential issues > > (as the rest of this patch). > > But isn't that like sweeping the problem under the rug? We'd still have > to test the different configurations (or let them rot), just that we'd > be testing different CFLAGS instead of configure flags. > Considering the direction of the discussion, I'll split the configure changes into 2 patches. One that allows glibc with _FORTIFY_SOURCE through whatever means, and a second one that introduces the new configure option. > Sid >
diff --git a/Makeconfig b/Makeconfig index 2514db35f6..59fbd9ebf9 100644 --- a/Makeconfig +++ b/Makeconfig @@ -543,12 +543,13 @@ endif # +link # ARM, gcc always produces different debugging symbols when invoked with # a -O greater than 0 than when invoked with -O0, regardless of anything else # we're using to suppress optimizations. Therefore, we need to explicitly pass -# -O0 to it through CFLAGS. +# -O0 to it through CFLAGS. By side effect, any fortification needs to be +# disabled as it needs -O greater than 0. # Additionally, the build system will try to -include $(common-objpfx)/config.h # when compiling the tests, which will throw an error if some special macros # (such as __OPTIMIZE__ and IS_IN_build) aren't defined. To avoid this, we # tell gcc to define IS_IN_build. -CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build +CFLAGS-printers-tests := -O0 -ggdb3 -DIS_IN_build $(no-fortify-source) ifeq (yes,$(build-shared)) # These indicate whether to link using the built ld.so or the installed one. @@ -901,6 +902,16 @@ define elide-stack-protector $(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-stack-protector)) endef +# We might want to compile with fortify-source +ifneq ($(fortify-source),) ++fortify-source=$(fortify-source) +endif + +# Some routine can't be fortified like the ones used by fortify +define elide-fortify-source +$(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-fortify-source)) +endef + # The program that makes Emacs-style TAGS files. ETAGS := etags @@ -961,6 +972,16 @@ endif # $(+cflags) == "" $(+stack-protector) -fno-common +gcc-nowarn := -w +# We must filter out elf because the early bootstrap of the dynamic loader +# cannot be fortified. Likewise we exclude dlfcn because it is entangled +# with the loader. We must filter out csu because early startup, like the +# loader, cannot be fortified. Lastly debug is the fortification routines +# themselves and they cannot be fortified. +do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) +ifeq ($(do-fortify),$(subdir)) ++cflags += $(+fortify-source) +endif + # Each sysdeps directory can contain header files that both will be # used to compile and will be installed. Each can also contain an # include/ subdirectory, whose header files will be used to compile @@ -1010,7 +1031,7 @@ module-cppflags = $(if $(filter %.mk.i %.v.i,$(@F)),,$(module-cppflags-real)) # Note that we can't use -std=* in CPPFLAGS, because it overrides # the implicit -lang-asm and breaks cpp behavior for .S files--notably # it causes cpp to stop predefining __ASSEMBLER__. -CPPFLAGS = $(config-extra-cppflags) $(CPPUNDEFS) $(CPPFLAGS-config) \ +CPPFLAGS = $(config-extra-cppflags) $(CPPFLAGS-config) \ $($(subdir)-CPPFLAGS) \ $(+includes) $(defines) $(module-cppflags) \ -include $(..)include/libc-symbols.h $(sysdep-CPPFLAGS) \ @@ -1049,6 +1070,8 @@ object-suffixes := CPPFLAGS-.o = $(pic-default) # libc.a must be compiled with -fPIE/-fpie for static PIE. CFLAGS-.o = $(filter %frame-pointer,$(+cflags)) $(pie-default) +CFLAGS-.o += $(call elide-fortify-source,.o,$(routines_no_fortify)) +CFLAGS-.o += $(call elide-fortify-source,_chk.o,$(routines_no_fortify)) libtype.o := lib%.a object-suffixes += .o ifeq (yes,$(build-shared)) @@ -1058,6 +1081,8 @@ object-suffixes += .os pic-cppflags = -DPIC -DSHARED CPPFLAGS-.os = $(pic-cppflags) CFLAGS-.os = $(filter %frame-pointer,$(+cflags)) $(pic-ccflag) +CFLAGS-.os += $(call elide-fortify-source,.os,$(routines_no_fortify)) +CFLAGS-.os += $(call elide-fortify-source,_chk.os,$(routines_no_fortify)) libtype.os := lib%_pic.a # This can be changed by a sysdep makefile pic-ccflag = -fPIC @@ -1077,6 +1102,8 @@ object-suffixes += .op CPPFLAGS-.op = -DPROF $(pic-default) # libc_p.a must be compiled with -fPIE/-fpie for static PIE. CFLAGS-.op = -pg $(pie-default) +CFLAGS-.op += $(call elide-fortify-source,.op,$(routines_no_fortify)) +CFLAGS-.op += $(call elide-fortify-source,_chk.op,$(routines_no_fortify)) libtype.op = lib%_p.a endif diff --git a/config.make.in b/config.make.in index 4afd37feaf..d487a4f4e9 100644 --- a/config.make.in +++ b/config.make.in @@ -64,6 +64,8 @@ have-fpie = @libc_cv_fpie@ have-ssp = @libc_cv_ssp@ stack-protector = @stack_protector@ no-stack-protector = @no_stack_protector@ +fortify-source = @fortify_source@ +no-fortify-source = @no_fortify_source@ have-selinux = @have_selinux@ have-libaudit = @have_libaudit@ have-libcap = @have_libcap@ @@ -101,7 +103,6 @@ CXX = @CXX@ BUILD_CC = @BUILD_CC@ CFLAGS = @CFLAGS@ CPPFLAGS-config = @CPPFLAGS@ -CPPUNDEFS = @CPPUNDEFS@ extra-nonshared-cflags = @extra_nonshared_cflags@ rtld-early-cflags = @rtld_early_cflags@ ASFLAGS-config = @ASFLAGS_config@ diff --git a/configure.ac b/configure.ac index 21879c933c..ec4de6e551 100644 --- a/configure.ac +++ b/configure.ac @@ -466,6 +466,17 @@ AC_ARG_ENABLE([scv], AS_IF([[test "$use_scv" != "no"]],[AC_DEFINE(USE_PPC_SCV)]) +dnl Build glibc with _FORTIFY_SOURCE +AC_ARG_ENABLE(fortify-source, + AS_HELP_STRING([--enable-fortify-source@<:@=1|2|3@:>@], + [Use -D_FORTIFY_SOURCE=[1|2|3] to control code hardening, defaults to highest possible value for your system]), + [enable_fortify_source=$enableval], + [enable_fortify_source=no]) +case "$enable_fortify_source" in +1|2|3|no|yes) ;; +*) AC_MSG_ERROR([Not a valid argument for --enable-fortify-source: "$enable_fortify_source"]);; +esac + # We keep the original values in `$config_*' and never modify them, so we # can write them unchanged into config.make. Everything else uses # $machine, $vendor, and $os, and changes them whenever convenient. @@ -1559,24 +1570,37 @@ if test "x$have_selinux" = xyes; then fi AC_SUBST(have_selinux) -CPPUNDEFS= -dnl Check for silly hacked compilers predefining _FORTIFY_SOURCE. -dnl Since we are building the implementations of the fortified functions here, -dnl having the macro defined interacts very badly. -dnl _FORTIFY_SOURCE requires compiler optimization level 1 (gcc -O1) -dnl and above (see "man FEATURE_TEST_MACROS"). -dnl So do NOT replace AC_COMPILE_IFELSE with AC_PREPROC_IFELSE. -AC_CACHE_CHECK([for _FORTIFY_SOURCE predefine], libc_cv_predef_fortify_source, -[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[]], [[ -#ifdef _FORTIFY_SOURCE -# error bogon -#endif]])], - [libc_cv_predef_fortify_source=no], - [libc_cv_predef_fortify_source=yes])]) -if test $libc_cv_predef_fortify_source = yes; then - CPPUNDEFS="${CPPUNDEFS:+$CPPUNDEFS }-U_FORTIFY_SOURCE" -fi -AC_SUBST(CPPUNDEFS) +dnl Check if we support the requested _FORTIFY_SOURCE level +dnl If not, then don't use it. +dnl Note that _FORTIFY_SOURCE may have been set through FLAGS too. +dnl _FORTIFY_SOURCE value will be selectively disabled for function that can't +dnl support it +fortify_source="" +no_fortify_source="-Wp,-U_FORTIFY_SOURCE" + +AC_CACHE_CHECK([for __builtin_dynamic_object_size], [libc_cv___builtin_dynamic_object_size], [ + AC_LINK_IFELSE([AC_LANG_PROGRAM([], [__builtin_dynamic_object_size("", 0)])], + [libc_cv___builtin_dynamic_object_size=yes + AS_IF([test "$enable_fortify_source" = yes], [enable_fortify_source=3])], + [libc_cv___builtin_dynamic_object_size=no + AS_IF([test "$enable_fortify_source" = yes], [enable_fortify_source=2])]) +]) + +AS_CASE([$enable_fortify_source], + [1|2], [libc_cv_fortify_source=yes], + [3], [AS_IF([test "$libc_cv___builtin_dynamic_object_size" = yes], + [libc_cv_fortify_source=yes], + [AC_MSG_ERROR([Compiler doesn't provide necessary support for _FORTIFY_SOURCE=3])])], + [libc_cv_fortify_source=no]) + +AS_IF([test "$libc_cv_fortify_source" = yes], + [fortify_source="${no_fortify_source},-D_FORTIFY_SOURCE=${enable_fortify_source}"] + ) + +AC_SUBST(enable_fortify_source) +AC_SUBST(libc_cv_fortify_source) +AC_SUBST(no_fortify_source) +AC_SUBST(fortify_source) dnl Starting with binutils 2.35, GAS can attach multiple symbol versions dnl to one symbol (PR 23840). diff --git a/elf/rtld-Rules b/elf/rtld-Rules index 56bc4543de..365a3408f3 100644 --- a/elf/rtld-Rules +++ b/elf/rtld-Rules @@ -144,6 +144,6 @@ cpp-srcs-left := $(rtld-modules:%.os=%) lib := rtld include $(patsubst %,$(..)libof-iterator.mk,$(cpp-srcs-left)) -rtld-CFLAGS += $(no-stack-protector) +rtld-CFLAGS += $(no-stack-protector) $(no-fortify-source) endif