diff mbox series

[3/6] package/Makefile.in: Use gcc spec files for PIE build flags

Message ID 20180711143113.11927-4-matthew.weber@rockwellcollins.com
State Changes Requested
Headers show
Series Hardening Flag Bugfix/Enhancement | expand

Commit Message

Matt Weber July 11, 2018, 2:31 p.m. UTC 
From: Stefan Sørensen <stefan.sorensen@spectralink.com>

The PIE build flags are only intended for building executables and can not be
used in relocateable links (-r), static builds and shared library build -
including the flags here causes build errors.

So instead of parsing the PIE flags directly on the command line to gcc,
include them in a gcc spec file where it is possible to only apply the flags
when other incompatible flags are not set.

This method and the spec files are from the Fedora build system.

Originally submitted as
http://patchwork.ozlabs.org/patch/907093/

Signed-off-by: Stefan Sørensen <stefan.sorensen@spectralink.com>
Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
---
 package/Makefile.in         | 4 ++--
 toolchain/gcc-specs-pie-cc1 | 2 ++
 toolchain/gcc-specs-pie-ld  | 2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)
 create mode 100644 toolchain/gcc-specs-pie-cc1
 create mode 100644 toolchain/gcc-specs-pie-ld

Comments

Arnout Vandecappelle July 11, 2018, 9:44 p.m. UTC | #1 
On 11-07-18 16:31, Matt Weber wrote:
> From: Stefan Sørensen <stefan.sorensen@spectralink.com>
> 
> The PIE build flags are only intended for building executables and can not be
> used in relocateable links (-r), static builds and shared library build -
> including the flags here causes build errors.
> 
> So instead of parsing the PIE flags directly on the command line to gcc,
> include them in a gcc spec file where it is possible to only apply the flags
> when other incompatible flags are not set.

 The commit message should also have the reasoning why this is considered better
than doing the same in the wrapper.

> 
> This method and the spec files are from the Fedora build system.
> 
> Originally submitted as
> http://patchwork.ozlabs.org/patch/907093/
> 
> Signed-off-by: Stefan Sørensen <stefan.sorensen@spectralink.com>
> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
> ---
>  package/Makefile.in         | 4 ++--
>  toolchain/gcc-specs-pie-cc1 | 2 ++
>  toolchain/gcc-specs-pie-ld  | 2 ++
>  3 files changed, 6 insertions(+), 2 deletions(-)
>  create mode 100644 toolchain/gcc-specs-pie-cc1
>  create mode 100644 toolchain/gcc-specs-pie-ld
> 
> diff --git a/package/Makefile.in b/package/Makefile.in
> index 14b3bbd243..00ddf48c10 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -158,8 +158,8 @@ ifeq ($(BR2_RELRO_PARTIAL),y)
>  TARGET_HARDENED += $(TARGET_CFLAGS_RELRO)
>  TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO)
>  else ifeq ($(BR2_RELRO_FULL),y)
> -TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
> -TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
> +TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
> +TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld

 As mentioned in my other mail, it is very likely that there are packages that
link with TARGET_CFLAGS instead of TARGET_LDFLAGS. Would that be a problem?
Actually, can't we just have a single spec file that contains both?


 Regards,
 Arnout

>  endif
>  
>  ifeq ($(BR2_FORTIFY_SOURCE_1),y)
> diff --git a/toolchain/gcc-specs-pie-cc1 b/toolchain/gcc-specs-pie-cc1
> new file mode 100644
> index 0000000000..fc54bcb510
> --- /dev/null
> +++ b/toolchain/gcc-specs-pie-cc1
> @@ -0,0 +1,2 @@
> +*cc1_options:
> ++ %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}
> diff --git a/toolchain/gcc-specs-pie-ld b/toolchain/gcc-specs-pie-ld
> new file mode 100644
> index 0000000000..bd6b9071ff
> --- /dev/null
> +++ b/toolchain/gcc-specs-pie-ld
> @@ -0,0 +1,2 @@
> +*self_spec:
> ++ %{!static:%{!shared:%{!r:-pie}}}
Matt Weber July 11, 2018, 11:17 p.m. UTC | #2 
Arnout,

On Wed, Jul 11, 2018 at 4:44 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
> On 11-07-18 16:31, Matt Weber wrote:
>> From: Stefan Sørensen <stefan.sorensen@spectralink.com>
>>
>> The PIE build flags are only intended for building executables and can not be
>> used in relocateable links (-r), static builds and shared library build -
>> including the flags here causes build errors.
>>
>> So instead of parsing the PIE flags directly on the command line to gcc,
>> include them in a gcc spec file where it is possible to only apply the flags
>> when other incompatible flags are not set.
>
>  The commit message should also have the reasoning why this is considered better
> than doing the same in the wrapper.
>

It probably isn't better then doing it in the wrapper.  At this point,
its just fixing something that's broken with the current approach.  I
was assuming a solid baseline option with this feature that's working
first and then start looking at replacing the approach with the
suggestion awhile back with doing it via the wrapper.


>> -TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
>> -TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
>> +TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
>> +TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld
>
>  As mentioned in my other mail, it is very likely that there are packages that
> link with TARGET_CFLAGS instead of TARGET_LDFLAGS. Would that be a problem?
> Actually, can't we just have a single spec file that contains both?

Like you mentioned previously, flag usage isn't consistent.  When we
started doing validation of the elf files we found that inconsistency
and we were individually trying to fix the packages.  This didn't get
to far as lots of the packages are in stable, no real maintaining
occuring.  I'm glad Stefan brought up the spec file idea as that
solved all the rework on the linker ordering and combination of
values.

Related to using a single spec file.  I'm not to familiar with using
them but since we have a mix of good/bad flag options already
occurring, guessing it wouldn't matter to consolidate to one file.  I
can test going to a single file. Stefan, do you know?

Matt
Arnout Vandecappelle July 13, 2018, 9:39 a.m. UTC | #3 
On 12-07-18 01:17, Matthew Weber wrote:
> Arnout,
> 
> On Wed, Jul 11, 2018 at 4:44 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>
>>
>> On 11-07-18 16:31, Matt Weber wrote:
>>> From: Stefan Sørensen <stefan.sorensen@spectralink.com>
>>>
>>> The PIE build flags are only intended for building executables and can not be
>>> used in relocateable links (-r), static builds and shared library build -
>>> including the flags here causes build errors.
>>>
>>> So instead of parsing the PIE flags directly on the command line to gcc,
>>> include them in a gcc spec file where it is possible to only apply the flags
>>> when other incompatible flags are not set.
>>
>>  The commit message should also have the reasoning why this is considered better
>> than doing the same in the wrapper.
>>
> 
> It probably isn't better then doing it in the wrapper.  At this point,
> its just fixing something that's broken with the current approach.  I
> was assuming a solid baseline option with this feature that's working
> first and then start looking at replacing the approach with the
> suggestion awhile back with doing it via the wrapper.

 I'm a bit confused about where you'd like to go in the long run: spec file or
wrapper? In your reply to the cover letter you mention "I do like the concept of
changing to the wrapper and evaluating if we can use GCC spec files"...

 I guess the spec files have the advantage that it is a lot easier to specify
mutually exclusive options. We have that now in the wrapper for arch/cpu/tune,
but it's a lot of code to specify something simple. That said, spec files are so
arcane that you can't expect many people contributing to it. The C code, though
more verbose, is a lot more understandable.

 Back to the patch: basically, the same could be done in the wrapper, but here
the spec files are just taken from Fedora so that's a lot simpler. Right?


>>> -TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
>>> -TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
>>> +TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
>>> +TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld
>>
>>  As mentioned in my other mail, it is very likely that there are packages that
>> link with TARGET_CFLAGS instead of TARGET_LDFLAGS. Would that be a problem?

 I don't think you answered this question?

>> Actually, can't we just have a single spec file that contains both?
> 
> Like you mentioned previously, flag usage isn't consistent.  When we
> started doing validation of the elf files we found that inconsistency
> and we were individually trying to fix the packages.  This didn't get
> to far as lots of the packages are in stable, no real maintaining
> occuring.  I'm glad Stefan brought up the spec file idea as that
> solved all the rework on the linker ordering and combination of
> values.

 Yes, spec file (or wrapper) is definitely better than patching individual packages.


 Regards,
 Arnout

> Related to using a single spec file.  I'm not to familiar with using
> them but since we have a mix of good/bad flag options already
> occurring, guessing it wouldn't matter to consolidate to one file.  I
> can test going to a single file. Stefan, do you know?
> 
> Matt
>
Matt Weber July 13, 2018, 12:31 p.m. UTC | #4 
Arnout,

On Fri, Jul 13, 2018 at 4:39 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
> On 12-07-18 01:17, Matthew Weber wrote:
>> Arnout,
>>
>> On Wed, Jul 11, 2018 at 4:44 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>>
>>>
>>> On 11-07-18 16:31, Matt Weber wrote:
>>>> From: Stefan Sørensen <stefan.sorensen@spectralink.com>
>>>>
>>>> The PIE build flags are only intended for building executables and can not be
>>>> used in relocateable links (-r), static builds and shared library build -
>>>> including the flags here causes build errors.
>>>>
>>>> So instead of parsing the PIE flags directly on the command line to gcc,
>>>> include them in a gcc spec file where it is possible to only apply the flags
>>>> when other incompatible flags are not set.
>>>
>>>  The commit message should also have the reasoning why this is considered better
>>> than doing the same in the wrapper.
>>>
>>
>> It probably isn't better then doing it in the wrapper.  At this point,
>> its just fixing something that's broken with the current approach.  I
>> was assuming a solid baseline option with this feature that's working
>> first and then start looking at replacing the approach with the
>> suggestion awhile back with doing it via the wrapper.
>
>  I'm a bit confused about where you'd like to go in the long run: spec file or
> wrapper? In your reply to the cover letter you mention "I do like the concept of
> changing to the wrapper and evaluating if we can use GCC spec files"...

Sorry, I was pulling a few thoughts together and didn't explain it
clearly.  I was thinking back to previous threads were it was
discussed to take all the hardening options and use the wrapper to
manage them vs directly setting the flag variables.  Ideally, I agree
that a wrapper approach for that is more ideal.  Since that thread,
there has been discussion on Stefan's patches about using spec files
as an option to simplify what the wrapper currently does.  However,
I'd agree that the spec syntax is complex.  For this patchset, the
spec file is accompishing a specific task of the mutually exclusive
options just related to hardening.

So long term, I could see the wrapper handing the hardening flag
management directly and the seperate possibility of spec files
possibly being used for other unrelated items that might simplify
wrapper logic.

>
>  I guess the spec files have the advantage that it is a lot easier to specify
> mutually exclusive options. We have that now in the wrapper for arch/cpu/tune,
> but it's a lot of code to specify something simple. That said, spec files are so
> arcane that you can't expect many people contributing to it. The C code, though
> more verbose, is a lot more understandable.

Agree.

>
>  Back to the patch: basically, the same could be done in the wrapper, but here
> the spec files are just taken from Fedora so that's a lot simpler. Right?

Correct.

>
>
>>>> -TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
>>>> -TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
>>>> +TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
>>>> +TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld
>>>
>>>  As mentioned in my other mail, it is very likely that there are packages that
>>> link with TARGET_CFLAGS instead of TARGET_LDFLAGS. Would that be a problem?
>
>  I don't think you answered this question?

Sorry lumped this in with the one below.  We found that flag usage was
inconsistent across packages and we were required to over-specify
flags.  So some packages that link correctly may get linker oriented
flags through both cflags and ldflags.  We verified that syntax wise
that this will work and that the post build elf file testing reflects
they take affect.

>
>>> Actually, can't we just have a single spec file that contains both?
>>
>> Like you mentioned previously, flag usage isn't consistent.  When we
>> started doing validation of the elf files we found that inconsistency
>> and we were individually trying to fix the packages.  This didn't get
>> to far as lots of the packages are in stable, no real maintaining
>> occuring.  I'm glad Stefan brought up the spec file idea as that
>> solved all the rework on the linker ordering and combination of
>> values.
>
>  Yes, spec file (or wrapper) is definitely better than patching individual packages.
>
>
>> Related to using a single spec file.  I'm not to familiar with using
>> them but since we have a mix of good/bad flag options already
>> occurring, guessing it wouldn't matter to consolidate to one file.  I
>> can test going to a single file. Stefan, do you know?

I've verified you have to have individual files for each.  Guessing
there isn't shared syntax between cc/ld spec files based on the errors
I got when the compiler was invoked with the additional self_spec
entry in it's spec file.  I couldn't fine enough documentation to
understand what that entry is doing.....

Matt
Sørensen, Stefan July 19, 2018, 9:49 a.m. UTC | #5 
On Wed, 2018-07-11 at 18:17 -0500, Matthew Weber wrote:
> Related to using a single spec file.  I'm not to familiar with using
> them but since we have a mix of good/bad flag options already
> occurring, guessing it wouldn't matter to consolidate to one file.  I
> can test going to a single file. Stefan, do you know?

It works fine with a single spec file - I used two different files
since I was keeping CFLAGS and LDFLAGS separate.

I will post an updated series with a single spec file and a few other
changes.

Stefan
Matt Weber July 19, 2018, 12:58 p.m. UTC | #6 
Stefan,

On Thu, Jul 19, 2018 at 4:49 AM, Sørensen, Stefan
<Stefan.Sorensen@spectralink.com> wrote:
>
> On Wed, 2018-07-11 at 18:17 -0500, Matthew Weber wrote:
> > Related to using a single spec file.  I'm not to familiar with using
> > them but since we have a mix of good/bad flag options already
> > occurring, guessing it wouldn't matter to consolidate to one file.  I
> > can test going to a single file. Stefan, do you know?
>
> It works fine with a single spec file - I used two different files
> since I was keeping CFLAGS and LDFLAGS separate.
>

Something in the syntax then would have to change, right?  I combined
the files and was getting CC errors.
Sørensen, Stefan July 19, 2018, 1:10 p.m. UTC | #7 
On Thu, 2018-07-19 at 07:58 -0500, Matthew Weber wrote:
> > It works fine with a single spec file - I used two different files
> > since I was keeping CFLAGS and LDFLAGS separate.
> > 
> 
> Something in the syntax then would have to change, right?  I combined
> the files and was getting CC errors.

That was what I did - I have not run into any problems. Any particular
packages that fails?

Stefan
Matt Weber Aug. 7, 2018, 5:02 p.m. UTC | #8 
On Thu, Jul 19, 2018 at 8:10 AM Sørensen, Stefan
<Stefan.Sorensen@spectralink.com> wrote:
>
> On Thu, 2018-07-19 at 07:58 -0500, Matthew Weber wrote:
> > > It works fine with a single spec file - I used two different files
> > > since I was keeping CFLAGS and LDFLAGS separate.
> > >
> >
> > Something in the syntax then would have to change, right?  I combined
> > the files and was getting CC errors.
>
> That was what I did - I have not run into any problems. Any particular
> packages that fails?

attr pkg during the configuring stage
https://paste.ubuntu.com/p/wdn3Pjsd39/

I can work up a simple defconfig but I didn't get to far before I ran into this.

The combined specs file looked like.
*cc1_options:
+ %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}
*self_spec:
+ %{!static:%{!shared:%{!r:-pie}}}

Matt
Matt Weber Aug. 7, 2018, 5:20 p.m. UTC | #9 
Stefan,
On Tue, Aug 7, 2018 at 12:02 PM Matthew Weber
<matthew.weber@rockwellcollins.com> wrote:
>
> On Thu, Jul 19, 2018 at 8:10 AM Sørensen, Stefan
> <Stefan.Sorensen@spectralink.com> wrote:
> >
> > On Thu, 2018-07-19 at 07:58 -0500, Matthew Weber wrote:
> > > > It works fine with a single spec file - I used two different files
> > > > since I was keeping CFLAGS and LDFLAGS separate.
> > > >
> > >
> > > Something in the syntax then would have to change, right?  I combined
> > > the files and was getting CC errors.
> >
> > That was what I did - I have not run into any problems. Any particular
> > packages that fails?
>
> attr pkg during the configuring stage
> https://paste.ubuntu.com/p/wdn3Pjsd39/
>
> I can work up a simple defconfig but I didn't get to far before I ran into this.
>
> The combined specs file looked like.
> *cc1_options:
> + %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}
> *self_spec:
> + %{!static:%{!shared:%{!r:-pie}}}
>

Config to reproduce.
BR2_aarch64=y
BR2_SSP_STRONG=y
BR2_RELRO_FULL=y
BR2_FORTIFY_SOURCE_2=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_TARGET_GENERIC_GETTY_PORT="ttyAMA0"
BR2_SYSTEM_DHCP="eth0"
BR2_LINUX_KERNEL=y
BR2_LINUX_KERNEL_CUSTOM_VERSION=y
BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="4.13.6"
BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y
BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/qemu/aarch64-virt/linux-4.13.config"
BR2_PACKAGE_ATTR=y
BR2_TARGET_ROOTFS_EXT2=y
BR2_TARGET_ROOTFS_EXT2_4=y
# BR2_TARGET_ROOTFS_TAR is not set
Jan Kundrát Aug. 8, 2018, 7:24 a.m. UTC | #10 
Hi Matt, thanks for working on this.

When I apply this patch, I'm getting a build failure from an out-of-tree 
[1] package, see the attached log. The package lives at [1], the Buildroot 
recipe is pretty straightforward [2]. Here's the relevant snippet from the 
failure:

  /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc -Wall 
-Wl,-z -Wl,now -Wl,-z -Wl,relro 
-specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld 
-o .libs/example example.o  ./.libs/libredblack.so
  
/home/jkt/work/prog/_build/br-cfb/host/opt/ext-toolchain/bin/../lib/gcc/arm-linux-gnueabihf/7.3.1/../../../../arm-linux-gnueabihf/bin/ld: 
example.o: relocation R_ARM_MOVW_ABS_NC against `stderr' can not be used 
when making a shared object; recompile with -fPIC

Is that a failure of that particular package, or a problem in the compiler 
specs?

Also, Gentoo Linux has been doing compiler hardening for years and their 
specs appear to be more complex according to the docs [4]. I have no clue 
if that extra complexity is needed in Buildroot.

With kind regards,
Jan

[1] I'll submit them once my patches reach a release upstream.
[2] https://github.com/sysrepo/libredblack
[3] 
https://gerrit.cesnet.cz/plugins/gitiles/github/buildroot/buildroot/+/a0cff08df5918e1721723400da42c8c68b8c0928%5E%21/
[4] https://wiki.gentoo.org/wiki/Hardened/Toolchain
$ make -j1 libredblack
umask 0022 && make -C /home/jkt/work/cesnet/gerrit/github/buildroot/buildroot O=/home/jkt/work/prog/_build/br-cfb/. libredblack
PATH="/home/jkt/work/prog/_build/br-cfb/host/bin:/home/jkt/work/prog/_build/br-cfb/host/sbin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0:/usr/lib/llvm/6/bin:/usr/lib/llvm/4/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/games/bin" BR2_DL_DIR="/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/dl" BUILD_DIR=/home/jkt/work/prog/_build/br-cfb/build O=/home/jkt/work/prog/_build/br-cfb flock /home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/dl/libredblack/ support/download/dl-wrapper -c '1.3' -d '/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/dl/libredblack' -D '/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/dl' -f 'libredblack-1.3.tar.gz' -H 'package/libredblack//libredblack.hash' -n 'libredblack-1.3' -N 'libredblack' -o '/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/dl/libredblack/libredblack-1.3.tar.gz'  -u http+http://downloads.sourceforge.net/project/libredblack/libredblack/1.3 -u http\|urlencode+http://sources.buildroot.net/libredblack -u http\|urlencode+http://sources.buildroot.net  -- 
libredblack-1.3.tar.gz: OK (sha256: a0ecc59b0aae2df01558a6950532c711a782a099277b439a51d270003092f44f)
>>> libredblack 1.3 Extracting
gzip -d -c /home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/dl/libredblack/libredblack-1.3.tar.gz | tar --strip-components=1 -C /home/jkt/work/prog/_build/br-cfb/build/libredblack-1.3   -xf -
>>> libredblack 1.3 Patching
>>> libredblack 1.3 Updating config.sub and config.guess
for file in config.guess config.sub; do for i in $(find /home/jkt/work/prog/_build/br-cfb/build/libredblack-1.3 -name $file); do cp support/gnuconfig/$file $i; done; done
>>> libredblack 1.3 Patching libtool
>>> libredblack 1.3 Configuring
(cd /home/jkt/work/prog/_build/br-cfb/build/libredblack-1.3/ && rm -rf config.cache && PATH="/home/jkt/work/prog/_build/br-cfb/host/bin:/home/jkt/work/prog/_build/br-cfb/host/sbin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0:/usr/lib/llvm/6/bin:/usr/lib/llvm/4/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/games/bin" AR="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-ar" AS="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-as" LD="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-ld" NM="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-nm" CC="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc" GCC="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc" CPP="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-cpp" CXX="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-g++" FC="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gfortran" F77="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gfortran" RANLIB="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-ranlib" READELF="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-readelf" STRIP="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-strip" OBJCOPY="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-objcopy" OBJDUMP="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-objdump" AR_FOR_BUILD="/usr/bin/ar" AS_FOR_BUILD="/usr/bin/as" CC_FOR_BUILD="/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0/gcc" GCC_FOR_BUILD="/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0/gcc" CXX_FOR_BUILD="/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0/g++" LD_FOR_BUILD="/usr/bin/ld" CPPFLAGS_FOR_BUILD="-I/home/jkt/work/prog/_build/br-cfb/host/include" CFLAGS_FOR_BUILD="-O2 -I/home/jkt/work/prog/_build/br-cfb/host/include" CXXFLAGS_FOR_BUILD="-O2 -I/home/jkt/work/prog/_build/br-cfb/host/include" LDFLAGS_FOR_BUILD="-L/home/jkt/work/prog/_build/br-cfb/host/lib -Wl,-rpath,/home/jkt/work/prog/_build/br-cfb/host/lib" FCFLAGS_FOR_BUILD="" DEFAULT_ASSEMBLER="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-as" DEFAULT_LINKER="/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-ld" CPPFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64" CFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -Os -g2 -fstack-protector-strong -Wl,-z,now -Wl,-z,relro -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-cc1 -D_FORTIFY_SOURCE=2" CXXFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -Os -g2 -fstack-protector-strong -Wl,-z,now -Wl,-z,relro -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-cc1 -D_FORTIFY_SOURCE=2" LDFLAGS=" -Wl,-z,now -Wl,-z,relro -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld" FCFLAGS=" -Os -g2" FFLAGS=" -Os -g2" PKG_CONFIG="/home/jkt/work/prog/_build/br-cfb/host/bin/pkg-config" STAGING_DIR="/home/jkt/work/prog/_build/br-cfb/host/arm-buildroot-linux-gnueabihf/sysroot" INTLTOOL_PERL=/usr/bin/perl ac_cv_lbl_unaligned_fail=yes ac_cv_func_mmap_fixed_mapped=yes ac_cv_func_memcmp_working=yes ac_cv_have_decl_malloc=yes gl_cv_func_malloc_0_nonnull=yes ac_cv_func_malloc_0_nonnull=yes ac_cv_func_calloc_0_nonnull=yes ac_cv_func_realloc_0_nonnull=yes lt_cv_sys_lib_search_path_spec="" ac_cv_c_bigendian=no   CONFIG_SITE=/dev/null ./configure --target=arm-buildroot-linux-gnueabihf --host=arm-buildroot-linux-gnueabihf --build=x86_64-pc-linux-gnu --prefix=/usr --exec-prefix=/usr --sysconfdir=/etc --localstatedir=/var --program-prefix="" --disable-gtk-doc --disable-gtk-doc-html --disable-doc --disable-docs --disable-documentation --with-xmlto=no --with-fop=no --disable-dependency-tracking --enable-ipv6 --disable-nls --disable-static --enable-shared  --with-rbgen=no )
configure: loading site script /dev/null
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking for arm-buildroot-linux-gnueabihf-strip... /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-strip
checking for arm-buildroot-linux-gnueabihf-gcc... /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc
checking for C compiler default output... a.out
checking whether the C compiler works... yes
checking whether we are cross compiling... yes
checking for suffix of executables... 
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc accepts -g... yes
checking for /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc option to accept ANSI C... none needed
checking for style of include used by make... GNU
checking dependency style of /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc... none
checking for a BSD-compatible install... /usr/bin/install -c
checking build system type... x86_64-pc-linux-gnu
checking host system type... arm-buildroot-linux-gnueabihf
checking for ld used by GCC... /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-ld
checking if the linker (/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-ld) is GNU ld... yes
checking for /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-ld option to reload object files... -r
checking for BSD-compatible nm... /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-nm
checking for a sed that does not truncate output... /bin/sed
checking whether ln -s works... yes
checking how to recognise dependent libraries... file_magic ELF [0-9][0-9]*-bit [LM]SB (shared object|dynamic lib )
checking command to parse /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-nm output... ok
checking how to run the C preprocessor... /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-cpp
checking for egrep... grep -E
checking for ANSI C header files... yes
checking for sys/types.h... yes
checking for sys/stat.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for memory.h... yes
checking for strings.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for unistd.h... yes
checking dlfcn.h usability... yes
checking dlfcn.h presence... yes
checking for dlfcn.h... yes
checking for arm-buildroot-linux-gnueabihf-file... no
checking for file... /usr/bin/file
checking for arm-buildroot-linux-gnueabihf-ranlib... /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-ranlib
checking for arm-buildroot-linux-gnueabihf-strip... (cached) /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-strip
checking for objdir... .libs
checking for /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc option to produce PIC... -fPIC
checking if /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc PIC flag -fPIC works... yes
checking if /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc static flag -static works... yes
checking if /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc supports -c -o file.o... yes
checking if /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc supports -c -o file.lo... yes
checking if /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc supports -fno-rtti -fno-exceptions... yes
checking whether the linker (/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-ld) supports shared libraries... yes
checking how to hardcode library paths into programs... immediate
checking whether stripping libraries is possible... yes
checking dynamic linker characteristics... GNU/Linux ld.so
checking if libtool supports shared libraries... yes
checking whether to build shared libraries... yes
checking whether to build static libraries... no
checking whether -lc should be explicitly linked in... no
creating libtool
checking for ANSI C header files... (cached) yes
checking for an ANSI C-conforming const... yes
checking for strdup... yes
configure: creating ./config.status
config.status: creating Makefile
config.status: creating libredblack.spec
config.status: creating rbgen
config.status: creating config.h
config.status: executing depfiles commands
config.status: executing default commands
>>> libredblack 1.3 Building
PATH="/home/jkt/work/prog/_build/br-cfb/host/bin:/home/jkt/work/prog/_build/br-cfb/host/sbin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0:/usr/lib/llvm/6/bin:/usr/lib/llvm/4/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/games/bin"  /usr/bin/make   -C /home/jkt/work/prog/_build/br-cfb/build/libredblack-1.3/
/usr/bin/make  all-am
/bin/sh ./libtool --mode=compile /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc -DHAVE_CONFIG_H -I. -I. -I.   -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -Wall -c -o redblack.lo `test -f 'redblack.c' || echo './'`redblack.c
/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc -DHAVE_CONFIG_H -I. -I. -I. -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Wall -c redblack.c  -fPIC -DPIC -o redblack.lo
redblack.c: In function 'rbinit':
redblack.c:143:7: warning: variable 'c' set but not used [-Wunused-but-set-variable]
  char c;
       ^
/bin/sh ./libtool --mode=link /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc  -Wall  -Wl,-z,now -Wl,-z,relro -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld -o libredblack.la -rpath /usr/lib -version-info 2:3:2 redblack.lo  
mkdir .libs
rm -fr .libs/libredblack.la .libs/libredblack.* .libs/libredblack.*
(cd . && ln -s redblack.lo redblack.o)
/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc -shared  redblack.lo   -Wl,-z -Wl,now -Wl,-z -Wl,relro -Wl,-soname -Wl,libredblack.so.0 -o .libs/libredblack.so.0.2.3
(cd .libs && rm -f libredblack.so.0 && ln -s libredblack.so.0.2.3 libredblack.so.0)
(cd .libs && rm -f libredblack.so && ln -s libredblack.so.0.2.3 libredblack.so)
creating libredblack.la
(cd .libs && rm -f libredblack.la && ln -s ../libredblack.la libredblack.la)
/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc -DHAVE_CONFIG_H -I. -I. -I.   -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -Wall -c `test -f 'example.c' || echo './'`example.c
/bin/sh ./libtool --mode=link /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc  -Wall  -Wl,-z,now -Wl,-z,relro -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld -o example  example.o libredblack.la 
/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc -Wall -Wl,-z -Wl,now -Wl,-z -Wl,relro -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld -o .libs/example example.o  ./.libs/libredblack.so
/home/jkt/work/prog/_build/br-cfb/host/opt/ext-toolchain/bin/../lib/gcc/arm-linux-gnueabihf/7.3.1/../../../../arm-linux-gnueabihf/bin/ld: example.o: relocation R_ARM_MOVW_ABS_NC against `stderr' can not be used when making a shared object; recompile with -fPIC
example.o: error adding symbols: Bad value
collect2: error: ld returned 1 exit status
make[3]: *** [Makefile:267: example] Error 1
make[2]: *** [Makefile:187: all] Error 2
make[1]: *** [package/pkg-generic.mk:232: /home/jkt/work/prog/_build/br-cfb/build/libredblack-1.3/.stamp_built] Error 2
make: *** [Makefile:16: _all] Error 2
Jan Kundrát Aug. 8, 2018, 8:35 a.m. UTC | #11 
I just got a similar failure from an in-tree package, the spi-tools:

>>> spi-tools 0.8.1 Building
PATH="/home/jkt/work/prog/_build/br-cfb/host/bin:/home/jkt/work/prog/_build/br-cfb/host/sbin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0:/usr/lib/llvm/6/bin:/usr/lib/llvm/4/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/games/bin" 
 /usr/bin/make   -C 
/home/jkt/work/prog/_build/br-cfb/build/spi-tools-0.8.1/
Making all in src
/usr/bin/make  all-am
/home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc  -O0  
-Wl,-z,now -Wl,-z,relro 
-specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld 
-o spi-config spi-config.o  
/home/jkt/work/prog/_build/br-cfb/host/opt/ext-toolchain/bin/../lib/gcc/arm-linux-gnueabihf/7.3.1/../../../../arm-linux-gnueabihf/bin/ld: 
spi-config.o: relocation R_ARM_MOVW_ABS_NC against `stderr' can not be used 
when making a shared object; recompile with -fPIC
spi-config.o: error adding symbols: Bad value
collect2: error: ld returned 1 exit status
Matt Weber Aug. 8, 2018, 11:38 a.m. UTC | #12 
Jan,

On Wed, Aug 8, 2018 at 3:36 AM Jan Kundrát <jan.kundrat@cesnet.cz> wrote:
>
> I just got a similar failure from an in-tree package, the spi-tools:

Cool, would you mind sharing your buildroot master hash + which of the
patchwork patches you've applied to it?  I'll recreate and take a
look.

Matt
Matt Weber Aug. 9, 2018, 2:32 p.m. UTC | #13 
Jan,
On Wed, Aug 8, 2018 at 3:36 AM Jan Kundrát <jan.kundrat@cesnet.cz> wrote:
>
> I just got a similar failure from an in-tree package, the spi-tools:
>
> >>> spi-tools 0.8.1 Building
> PATH="/home/jkt/work/prog/_build/br-cfb/host/bin:/home/jkt/work/prog/_build/br-cfb/host/sbin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0:/usr/lib/llvm/6/bin:/usr/lib/llvm/4/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/games/bin"
>  /usr/bin/make   -C
> /home/jkt/work/prog/_build/br-cfb/build/spi-tools-0.8.1/
> Making all in src
> /usr/bin/make  all-am
> /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc  -O0
> -Wl,-z,now -Wl,-z,relro
> -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld
> -o spi-config spi-config.o
> /home/jkt/work/prog/_build/br-cfb/host/opt/ext-toolchain/bin/../lib/gcc/arm-linux-gnueabihf/7.3.1/../../../../arm-linux-gnueabihf/bin/ld:
> spi-config.o: relocation R_ARM_MOVW_ABS_NC against `stderr' can not be used
> when making a shared object; recompile with -fPIC
> spi-config.o: error adding symbols: Bad value
> collect2: error: ld returned 1 exit status

This one has to do with the configure.ac of that package not observing
cflags/cxxflags being passed to configure.  So the *.o that are trying
to be linked weren't build as position independent.  Probably a
similar case for your libredblack package.

I've posted an upstream issue at this point and a basic fix.  As
buildroot could probably use a bump, rather then carrying the patch,
I'll ask upstream if they could do a tag after this fix is merged.
https://github.com/cpb-/spi-tools/issues/12

Matt
Thomas Petazzoni Aug. 10, 2018, 8:50 p.m. UTC | #14 
Matt, Stefan,

On Wed, 11 Jul 2018 09:31:10 -0500, Matt Weber wrote:
> From: Stefan Sørensen <stefan.sorensen@spectralink.com>
> 
> The PIE build flags are only intended for building executables and can not be
> used in relocateable links (-r), static builds and shared library build -
> including the flags here causes build errors.
> 
> So instead of parsing the PIE flags directly on the command line to gcc,
> include them in a gcc spec file where it is possible to only apply the flags
> when other incompatible flags are not set.
> 
> This method and the spec files are from the Fedora build system.
> 
> Originally submitted as
> http://patchwork.ozlabs.org/patch/907093/
> 
> Signed-off-by: Stefan Sørensen <stefan.sorensen@spectralink.com>
> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>

I've read the whole discussion on this patch, but still I don't
understand which direction we want to take moving forward: move away
from handling options in the wrapper, and use a spec file for
everything ? Or keep the entire logic in the wrapper ?

I'm not happy at all with the approach of having some flags handled in
the wrapper, some flags handled through spec files. I believe choosing
the spec file direction makes this patch series more difficult to
merge, because we have to go through this whole discussion of spec file
vs. wrapper.

I have nothing against using spec files, but right now, our logic is
based on a wrapper program. Therefore, I would be more comfortable with
an approach that relies on the wrapper program, so that it is in line
with what we are doing today. Then, separately, we can discuss how our
wrapper can be replaced, completely or partially, by a spec file.

Best regards,

Thomas
Matt Weber Aug. 11, 2018, 12:42 a.m. UTC | #15 
Thomas,
On Fri, Aug 10, 2018 at 3:50 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Matt, Stefan,
>
> On Wed, 11 Jul 2018 09:31:10 -0500, Matt Weber wrote:
> > From: Stefan Sørensen <stefan.sorensen@spectralink.com>
> >
> > The PIE build flags are only intended for building executables and can not be
> > used in relocateable links (-r), static builds and shared library build -
> > including the flags here causes build errors.
> >
> > So instead of parsing the PIE flags directly on the command line to gcc,
> > include them in a gcc spec file where it is possible to only apply the flags
> > when other incompatible flags are not set.
> >
> > This method and the spec files are from the Fedora build system.
> >
> > Originally submitted as
> > http://patchwork.ozlabs.org/patch/907093/
> >
> > Signed-off-by: Stefan Sørensen <stefan.sorensen@spectralink.com>
> > Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com>
>
> I've read the whole discussion on this patch, but still I don't
> understand which direction we want to take moving forward: move away
> from handling options in the wrapper, and use a spec file for
> everything ? Or keep the entire logic in the wrapper ?

So far, the spec files are just fixing a conditional case where a
option isn't compatible.  ie PIE and shared can't be used together.
This prevents a lot of patchwork on all the packages.  I could move
this string search and condition process to the wrapper but it seems
cleaner to do that in the spec file.  I do want to note that we did
not go to the extent that gentoo did where they use spec files to
enable and do the conditional compatibility fixups at link time(PIE vs
shared) for all the hardening options.

>
> I'm not happy at all with the approach of having some flags handled in
> the wrapper, some flags handled through spec files. I believe choosing
> the spec file direction makes this patch series more difficult to
> merge, because we have to go through this whole discussion of spec file
> vs. wrapper.

Agree, the specfile even just doing the conditional fixup is a 3rd leg
on this whole thing.  We can add it to the wrapper code but we will
loose visibility to what the wrapper is doing vs with the spec we can
dump and analyze the output.

>
> I have nothing against using spec files, but right now, our logic is
> based on a wrapper program. Therefore, I would be more comfortable with
> an approach that relies on the wrapper program, so that it is in line
> with what we are doing today. Then, separately, we can discuss how our
> wrapper can be replaced, completely or partially, by a spec file.

Ok. If the above feedback in this email doesn't change the view
related to spec files, I can look at switching to a pure wrapper for
fixups and static setting of variables with flags.


Matt
Thomas Petazzoni Aug. 11, 2018, 10:29 a.m. UTC | #16 
Hello,

On Fri, 10 Aug 2018 19:42:32 -0500, Matthew Weber wrote:

> > I've read the whole discussion on this patch, but still I don't
> > understand which direction we want to take moving forward: move away
> > from handling options in the wrapper, and use a spec file for
> > everything ? Or keep the entire logic in the wrapper ?  
> 
> So far, the spec files are just fixing a conditional case where a
> option isn't compatible.  ie PIE and shared can't be used together.
> This prevents a lot of patchwork on all the packages.  I could move
> this string search and condition process to the wrapper but it seems
> cleaner to do that in the spec file.  I do want to note that we did
> not go to the extent that gentoo did where they use spec files to
> enable and do the conditional compatibility fixups at link time(PIE vs
> shared) for all the hardening options.

I understand the idea of using the spec file. However, I'm worried that
it further complicates the overall toolchain setup. In addition, the
-spec option passed in TARGET_CFLAGS/TARGET_LDFLAGS will only work with
packages that do obey to CFLAGS/LDFLAGS, and some broken packages
don't. Being in the wrapper allows to ensure the flags are used. And
additionally, what's done in the wrapper is also used when the
Buildroot toolchain is used "manually" (outside of the Buildroot
infrastructure) to build stuff.

> > I'm not happy at all with the approach of having some flags handled in
> > the wrapper, some flags handled through spec files. I believe choosing
> > the spec file direction makes this patch series more difficult to
> > merge, because we have to go through this whole discussion of spec file
> > vs. wrapper.  
> 
> Agree, the specfile even just doing the conditional fixup is a 3rd leg
> on this whole thing.  We can add it to the wrapper code but we will
> loose visibility to what the wrapper is doing vs with the spec we can
> dump and analyze the output.

That's not true: the wrapper looks at BR2_DEBUG_WRAPPER={1,2} to dump
what it is doing:

        /* Debug the wrapper to see actual arguments passed to
         * the compiler:
         * unset, empty, or 0: do not trace
         * set to 1          : trace all arguments on a single line
         * set to 2          : trace one argument per line
         */
        if ((env_debug = getenv("BR2_DEBUG_WRAPPER"))) {

> > I have nothing against using spec files, but right now, our logic is
> > based on a wrapper program. Therefore, I would be more comfortable with
> > an approach that relies on the wrapper program, so that it is in line
> > with what we are doing today. Then, separately, we can discuss how our
> > wrapper can be replaced, completely or partially, by a spec file.  
> 
> Ok. If the above feedback in this email doesn't change the view
> related to spec files, I can look at switching to a pure wrapper for
> fixups and static setting of variables with flags.

I think I'd be happier with a first step where the wrapper is being
used. Then in a second step, we can decide to rethink what the wrapper
is doing, and move more of its logic to a spec file. I think a wrapper
will still be needed anyway, at least to pass --sysroot and --spec.

Best regards,

Thomas
Matt Weber Aug. 12, 2018, 3:55 a.m. UTC | #17 
Thomas,
On Sat, Aug 11, 2018 at 5:30 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Fri, 10 Aug 2018 19:42:32 -0500, Matthew Weber wrote:
>
> > > I've read the whole discussion on this patch, but still I don't
> > > understand which direction we want to take moving forward: move away
> > > from handling options in the wrapper, and use a spec file for
> > > everything ? Or keep the entire logic in the wrapper ?
> >
> > So far, the spec files are just fixing a conditional case where a
> > option isn't compatible.  ie PIE and shared can't be used together.
> > This prevents a lot of patchwork on all the packages.  I could move
> > this string search and condition process to the wrapper but it seems
> > cleaner to do that in the spec file.  I do want to note that we did
> > not go to the extent that gentoo did where they use spec files to
> > enable and do the conditional compatibility fixups at link time(PIE vs
> > shared) for all the hardening options.
>
> I understand the idea of using the spec file. However, I'm worried that
> it further complicates the overall toolchain setup. In addition, the
> -spec option passed in TARGET_CFLAGS/TARGET_LDFLAGS will only work with
> packages that do obey to CFLAGS/LDFLAGS, and some broken packages
> don't. Being in the wrapper allows to ensure the flags are used. And
> additionally, what's done in the wrapper is also used when the
> Buildroot toolchain is used "manually" (outside of the Buildroot
> infrastructure) to build stuff.
>
> > > I'm not happy at all with the approach of having some flags handled in
> > > the wrapper, some flags handled through spec files. I believe choosing
> > > the spec file direction makes this patch series more difficult to
> > > merge, because we have to go through this whole discussion of spec file
> > > vs. wrapper.
> >
> > Agree, the specfile even just doing the conditional fixup is a 3rd leg
> > on this whole thing.  We can add it to the wrapper code but we will
> > loose visibility to what the wrapper is doing vs with the spec we can
> > dump and analyze the output.
>
> That's not true: the wrapper looks at BR2_DEBUG_WRAPPER={1,2} to dump
> what it is doing:
>
>         /* Debug the wrapper to see actual arguments passed to
>          * the compiler:
>          * unset, empty, or 0: do not trace
>          * set to 1          : trace all arguments on a single line
>          * set to 2          : trace one argument per line
>          */
>         if ((env_debug = getenv("BR2_DEBUG_WRAPPER"))) {
>
> > > I have nothing against using spec files, but right now, our logic is
> > > based on a wrapper program. Therefore, I would be more comfortable with
> > > an approach that relies on the wrapper program, so that it is in line
> > > with what we are doing today. Then, separately, we can discuss how our
> > > wrapper can be replaced, completely or partially, by a spec file.
> >
> > Ok. If the above feedback in this email doesn't change the view
> > related to spec files, I can look at switching to a pure wrapper for
> > fixups and static setting of variables with flags.
>
> I think I'd be happier with a first step where the wrapper is being
> used. Then in a second step, we can decide to rethink what the wrapper
> is doing, and move more of its logic to a spec file. I think a wrapper
> will still be needed anyway, at least to pass --sysroot and --spec.
>

I started to look at this tonight and if I understand the current
wrapper, it's solely working with cc/gcc.  If someone explicity uses
ld, the wrapper doesn't take affect? (I'll do a build tomorrow and
look at behavior, didn't have a linux machine handy tonight to check.

If that's the case, any pointers to detecting the type of invocation
of the wrapper if I update the symlinking so ld also goes through?

Matt
Thomas Petazzoni Aug. 12, 2018, 7:41 a.m. UTC | #18 
Hello,

On Sat, 11 Aug 2018 22:55:47 -0500, Matthew Weber wrote:

> I started to look at this tonight and if I understand the current
> wrapper, it's solely working with cc/gcc.  If someone explicity uses
> ld, the wrapper doesn't take affect? (I'll do a build tomorrow and
> look at behavior, didn't have a linux machine handy tonight to check.

We only wrap gcc/cc indeed. Packages should not call "ld" directly, as
it breaks a bunch of other situations. For example, on mips64, using
"ld" directly doesn't work as it needs to be passed the "machine
emulation", which gcc does.

So I don't think we need to wrap "ld", as ld shouldn't be used
directly. The only packages that should use "ld" directly are things
like the Linux kernel or bootloaders.

Best regards,

Thomas
Matt Weber Aug. 12, 2018, 12:49 p.m. UTC | #19 
Thomas,
On Sun, Aug 12, 2018 at 2:41 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Sat, 11 Aug 2018 22:55:47 -0500, Matthew Weber wrote:
>
> > I started to look at this tonight and if I understand the current
> > wrapper, it's solely working with cc/gcc.  If someone explicity uses
> > ld, the wrapper doesn't take affect? (I'll do a build tomorrow and
> > look at behavior, didn't have a linux machine handy tonight to check.
>
> We only wrap gcc/cc indeed. Packages should not call "ld" directly, as
> it breaks a bunch of other situations. For example, on mips64, using
> "ld" directly doesn't work as it needs to be passed the "machine
> emulation", which gcc does.
>
> So I don't think we need to wrap "ld", as ld shouldn't be used
> directly. The only packages that should use "ld" directly are things
> like the Linux kernel or bootloaders.
>

The current hardening approach is trying to cover the cases where
packages are still using ld directly and have other incompatible flags
set (static/shared/r).  I don't have the exact list but I believe
busybox is even one of those and others like valgrind, boost, etc who
use the "shared" flag and adding "pie" causes a compile failure.  So
we do still need to cover the ld case or go patch packages.

What I could do is move the cc1 spec file conditional add of PIE into
the wrapper.  Then leave the LDFLAGS as we have them and the
associated spec file that does a conditional add of "pie".  This would
prevent us from wrapping the ld tool and keep the existing wrapper
approach consistent.

Matt
Thomas Petazzoni Aug. 12, 2018, 3:07 p.m. UTC | #20 
Hello,

On Sun, 12 Aug 2018 07:49:19 -0500, Matthew Weber wrote:

> > So I don't think we need to wrap "ld", as ld shouldn't be used
> > directly. The only packages that should use "ld" directly are things
> > like the Linux kernel or bootloaders.
> 
> The current hardening approach is trying to cover the cases where
> packages are still using ld directly and have other incompatible flags
> set (static/shared/r).  I don't have the exact list but I believe
> busybox is even one of those and others like valgrind, boost, etc who
> use the "shared" flag and adding "pie" causes a compile failure.  So
> we do still need to cover the ld case or go patch packages.

I find it weird that those packages are using "ld" directly, because if
that's the case, we would have build failures on some mips64
configurations.

> What I could do is move the cc1 spec file conditional add of PIE into
> the wrapper.  Then leave the LDFLAGS as we have them and the
> associated spec file that does a conditional add of "pie".  This would
> prevent us from wrapping the ld tool and keep the existing wrapper
> approach consistent.

If we really need to do some custom logic around ld, then I believe I'd
prefer to have a wrapper for it as well, to keep things consistent. But
of course, Arnout's opinion on the matter would be welcome.

Thomas
Arnout Vandecappelle Aug. 12, 2018, 9:20 p.m. UTC | #21 
On 12-08-18 17:07, Thomas Petazzoni wrote:
> Hello,
> 
> On Sun, 12 Aug 2018 07:49:19 -0500, Matthew Weber wrote:
> 
>>> So I don't think we need to wrap "ld", as ld shouldn't be used
>>> directly. The only packages that should use "ld" directly are things
>>> like the Linux kernel or bootloaders.
>>
>> The current hardening approach is trying to cover the cases where
>> packages are still using ld directly and have other incompatible flags
>> set (static/shared/r).  I don't have the exact list but I believe
>> busybox is even one of those and others like valgrind, boost, etc who
>> use the "shared" flag and adding "pie" causes a compile failure.  So
>> we do still need to cover the ld case or go patch packages.
> 
> I find it weird that those packages are using "ld" directly, because if
> that's the case, we would have build failures on some mips64
> configurations.

 In addition, if you call ld directly, the spec file is not used at all... The
spec file is only used by the gcc wrapper.


>> What I could do is move the cc1 spec file conditional add of PIE into
>> the wrapper.  Then leave the LDFLAGS as we have them and the
>> associated spec file that does a conditional add of "pie".  This would
>> prevent us from wrapping the ld tool and keep the existing wrapper
>> approach consistent.

 This indicates the crux of the problem with the wrapper: it is not easy to
detect in the wrapper that we're linking. This is "solved" by specifying the
linker spec file in LDFLAGS, but since many packages (probably) don't use
LDFLAGS, it is not really solved at all... Well, except in the sense that if a
package doesn't look at LDFLAGS, it's not going to get the -pie hardening flag
either so the problem that this patch is fixing will not occur.

 We *can* keep passing -pie in LDFLAGS, and then remove it again in the wrapper
when -shared or -r or -static is present, but it feels weird... Hm, but
apparently we can just always pass -pie in the wrapper, even when we're not
linking. That would solve the issue relatively elegantly within the wrapper.


> If we really need to do some custom logic around ld, then I believe I'd
> prefer to have a wrapper for it as well, to keep things consistent. But
> of course, Arnout's opinion on the matter would be welcome.

 I don't think an ld wrapper is needed. We've discussed this many times already,
and I see less and less reason for it.

 Regards,
 Arnout

> 
> Thomas
>
Matt Weber Aug. 28, 2018, 8:07 p.m. UTC | #22 
Jan,
On Wed, Aug 8, 2018 at 3:36 AM Jan Kundrát <jan.kundrat@cesnet.cz> wrote:
>
> I just got a similar failure from an in-tree package, the spi-tools:
>
> >>> spi-tools 0.8.1 Building
> PATH="/home/jkt/work/prog/_build/br-cfb/host/bin:/home/jkt/work/prog/_build/br-cfb/host/sbin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/home/jkt/.local/bin:/home/jkt/bin/:/opt/qtc/bin:/opt/darktable/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/8.1.0:/usr/lib/llvm/6/bin:/usr/lib/llvm/4/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/games/bin"
>  /usr/bin/make   -C
> /home/jkt/work/prog/_build/br-cfb/build/spi-tools-0.8.1/
> Making all in src
> /usr/bin/make  all-am
> /home/jkt/work/prog/_build/br-cfb/host/bin/arm-linux-gnueabihf-gcc  -O0
> -Wl,-z,now -Wl,-z,relro
> -specs=/home/jkt/work/cesnet/gerrit/github/buildroot/buildroot/toolchain/gcc-specs-pie-ld
> -o spi-config spi-config.o
> /home/jkt/work/prog/_build/br-cfb/host/opt/ext-toolchain/bin/../lib/gcc/arm-linux-gnueabihf/7.3.1/../../../../arm-linux-gnueabihf/bin/ld:
> spi-config.o: relocation R_ARM_MOVW_ABS_NC against `stderr' can not be used
> when making a shared object; recompile with -fPIC
> spi-config.o: error adding symbols: Bad value
> collect2: error: ld returned 1 exit status

I was able to get the spi-tools cflags not passing through from
buildroot fix upstreamed and new release from spi-tools.  Just sent a
bump to buildroot.

http://patchwork.ozlabs.org/patch/963100/
diff mbox series

Patch

diff --git a/package/Makefile.in b/package/Makefile.in
index 14b3bbd243..00ddf48c10 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -158,8 +158,8 @@  ifeq ($(BR2_RELRO_PARTIAL),y)
 TARGET_HARDENED += $(TARGET_CFLAGS_RELRO)
 TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO)
 else ifeq ($(BR2_RELRO_FULL),y)
-TARGET_HARDENED += -fPIE $(TARGET_CFLAGS_RELRO_FULL)
-TARGET_LDFLAGS += -pie $(TARGET_CFLAGS_RELRO_FULL)
+TARGET_HARDENED += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-cc1
+TARGET_LDFLAGS += $(TARGET_CFLAGS_RELRO_FULL) -specs=$(TOPDIR)/toolchain/gcc-specs-pie-ld
 endif
 
 ifeq ($(BR2_FORTIFY_SOURCE_1),y)
diff --git a/toolchain/gcc-specs-pie-cc1 b/toolchain/gcc-specs-pie-cc1
new file mode 100644
index 0000000000..fc54bcb510
--- /dev/null
+++ b/toolchain/gcc-specs-pie-cc1
@@ -0,0 +1,2 @@ 
+*cc1_options:
++ %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}
diff --git a/toolchain/gcc-specs-pie-ld b/toolchain/gcc-specs-pie-ld
new file mode 100644
index 0000000000..bd6b9071ff
--- /dev/null
+++ b/toolchain/gcc-specs-pie-ld
@@ -0,0 +1,2 @@ 
+*self_spec:
++ %{!static:%{!shared:%{!r:-pie}}}