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

Message ID 20180711143113.11927-4-matthew.weber@rockwellcollins.com
State New
Headers show
Series
  • Hardening Flag Bugfix/Enhancement
Related show

Commit Message

Matthew Weber July 11, 2018, 2:31 p.m.
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. | #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}}}
Matthew Weber July 11, 2018, 11:17 p.m. | #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. | #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
>
Matthew Weber July 13, 2018, 12:31 p.m. | #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. | #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
Matthew Weber July 19, 2018, 12:58 p.m. | #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. | #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

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}}}