Hi,

On Tue, Dec 18, 2018 at 08:03:54AM -0800, William Roberts wrote:
> Patrick,
> 
> Hoping you could maybe weigh in on your choice for bypassing the
> compiler driver with -Wp and not setting _FORTIFY_SOURCE to something
> like 1 or 2?
> 
> I'm seeing this issue on Ubuntu 16.04.5:
> <command-line>:0:0: error: "_FORTIFY_SOURCE" redefined [-Werror]
> 
> gcc version:
> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609
> 
> My thought is to undef/redef _FORTIFY_SOURCE in CFLAGS and set the
> level to 2. Setting CFLAGS via the env/make arg will override this
> behavior
> and use CFLAGS as is.

I used "-Wp" simply because it was existing previously, so I just
stuck to what was there already. The original issue I had was
that Gentoo Hardened, as Jason notes, already defines
_FORTIFY_SOURCE as part of the compiler spec. Due to that, I was
seeing a lot of warnings.

So I set the flag to a simple define without setting a specific
value, which _seemed_ to let the issue go away. But going back to
the initial issue, this didn't seem to have solved it correctly.
Dunno what I've been doing back then to not see the warnings
after my change anymore, but I noticed that they have resurface
recently.

So I guess the real fix would be to redefine the value by first
undef'ing it and then redefining it to the desired value. And I
do agree that in that case, we should simply revert to
_FORTIFY_SOURCE=2.

Patrick

> On Fri, Dec 14, 2018 at 8:02 AM William Roberts
> <bill.c.roberts@gmail.com> wrote:
> >
> > On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > >
> > > On 12/14/18 8:43 AM, Stephen Smalley wrote:
> > > > On 12/13/18 4:32 PM, bill.c.roberts@gmail.com wrote:
> > > >> From: William Roberts <william.c.roberts@intel.com>
> > > >>
> > > >> Certain builds of gcc enable _FORTIFY_SOURCE which results in the error:
> > > >> <command-line>:0:0: warning: "_FORTIFY_SOURCE" redefined
> > > >> <command-line>:0:0: note: this is the location of the previous definition
> > > >>
> > > >> Correct this by undefining it first and redefining it. Also, the previous
> > > >> command line option was using -Wp which passing the value *AS IS* to the
> > > >> pre-processor rather than to the compiler driver. The C pre-processor has
> > > >> an undocumented interface subject to change per man 1 gcc. Just use the
> > > >> -D option as is.
> > > >
> > > > See commit ca07a2ad46be141dad90d885dd33a2ac31c6559a ("libselinux: avoid
> > > > redefining _FORTIFY_SOURCE") for why we don't specify a value for
> > > > _FORTIFY_SOURCE here.  Not sure about the -Wp,-D vs -D rationale.
> >
> > I'm not 100% convinced that the patch is the best solution or the commit message
> > is describing the problem correctly. I could also be understanding it
> > wrong here.
> > The man page is saying not to bypass the compiler driver via -Wp, and I don't
> > see a good reason for it either.
> >
> > See my comments below, they feed back into this.
> >
> > >
> > > I guess the issue here is that we want to provide sane defaults for
> > > building without breaking the build when others specify their own
> > > definitions and without weakening those definitions.  By undefining and
> > > re-defining, it seems like we might weaken existing builds that were
> > > specifying 2.
> >
> > We conditionally assign to CFLAGS via ?= operator. Thus, CFLAGS and the
> > corresponding addition of EXTRA_CFLAGS which contains the undef/def
> > is not appended. CFLAGS specified via the environment or as an argument to
> > make will cause this assignment not to occur (via ?= semantics) and whatever
> > they specify for CFLAGS is sent to CC.
> >
> > Here is some sample output:
> > make CFLAGS='-D_FORTIFY_SOURCE=2'
> > cc -D_FORTIFY_SOURCE=2 -I../include -D_GNU_SOURCE -DNO_ANDROID_BACKEND
> >  -fPIC -DSHARED -c -o stringrep.lo stringrep.c
> >
> > With that said, *i think its safe* to bump it back to '-D_FORTIFY_SOURCE=2'
> >
> > >
> > > >
> > > >>
> > > >> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> > > >> ---
> > > >>   libselinux/src/Makefile   | 2 +-
> > > >>   libselinux/utils/Makefile | 2 +-
> > > >>   2 files changed, 2 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
> > > >> index 977b5c8cfcca..ee55bd0dbff7 100644
> > > >> --- a/libselinux/src/Makefile
> > > >> +++ b/libselinux/src/Makefile
> > > >> @@ -64,7 +64,7 @@ ifeq ($(COMPILER), gcc)
> > > >>   EXTRA_CFLAGS = -fipa-pure-const -Wlogical-op
> > > >> -Wpacked-bitfield-compat -Wsync-nand \
> > > >>       -Wcoverage-mismatch -Wcpp -Wformat-contains-nul -Wnormalized=nfc
> > > >> -Wsuggest-attribute=const \
> > > >>       -Wsuggest-attribute=noreturn -Wsuggest-attribute=pure
> > > >> -Wtrampolines -Wjump-misses-init \
> > > >> -    -Wno-suggest-attribute=pure -Wno-suggest-attribute=const
> > > >> -Wp,-D_FORTIFY_SOURCE
> > > >> +    -Wno-suggest-attribute=pure -Wno-suggest-attribute=const
> > > >> -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
> > > >>   else
> > > >>   EXTRA_CFLAGS = -Wunused-command-line-argument
> > > >>   endif
> > > >> diff --git a/libselinux/utils/Makefile b/libselinux/utils/Makefile
> > > >> index d06ffd66893b..64ab877015c6 100644
> > > >> --- a/libselinux/utils/Makefile
> > > >> +++ b/libselinux/utils/Makefile
> > > >> @@ -30,7 +30,7 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k
> > > >> -Wformat-security -Winit-self -Wmissi
> > > >>             -Wformat-extra-args -Wformat-zero-length -Wformat=2
> > > >> -Wmultichar \
> > > >>             -Woverflow -Wpointer-to-int-cast -Wpragmas \
> > > >>             -Wno-missing-field-initializers -Wno-sign-compare \
> > > >> -          -Wno-format-nonliteral
> > > >> -Wframe-larger-than=$(MAX_STACK_SIZE) -Wp,-D_FORTIFY_SOURCE \
> > > >> +          -Wno-format-nonliteral
> > > >> -Wframe-larger-than=$(MAX_STACK_SIZE) -U_FORTIFY_SOURCE
> > > >> -D_FORTIFY_SOURCE=1 \
> > > >>             -fstack-protector-all --param=ssp-buffer-size=4
> > > >> -fexceptions \
> > > >>             -fasynchronous-unwind-tables -fdiagnostics-show-option
> > > >> -funit-at-a-time \
> > > >>             -Werror -Wno-aggregate-return -Wno-redundant-decls \