Re: [PATCH 01/21] Add --enable-fortify-source option

[Frederic Berat <fberat@redhat.com>]

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
>