[v3,1/4] toolchain/toolchain-wrapper: add BR2_RELRO_FULL support

The current implementation is broken for full RELRO as the conditional
checking of flag combinations both for compile and link time need
to occur at the point of use in a package (GCC/LD invocation using
specfile) or be enforced via a toolchain wrapper GCC/LD args adjustment.
If the current approach is maintained, a large amount of packages will
need to be patched and made aware of this scenario or at a minimum
reorder their flag usage to be compatible.  I've found this type of
patch to be hard to upstream and in some cases touch makefiles that
haven't required updates on stable (unmaintained) projects.

There have been two main approaches for a solution (specfile vs
toolchain wrapper).

The specfile approach proposed by Stefan is a working solution and
has been tested by me with a few different target package configurations
as well as an internal autobuilder regressioning this feature.  This
solution isn't ideal as it adds another way Buildroot is adjusting flags
that may confuse/conflict with the toolchain wrapper approach.
(Ref Stefan's patch: http://patchwork.ozlabs.org/patch/942524/)

The second option was the addition of a new linker wrapper but after
completing that approach, the amount of complexity and new software,
seems to out weigh possibly doing a hybrid approach proposed in this
patchset.  The one main complexity of solely using wrappers, is the
fact that the GCC wrapper needs to know if it is being called with
LDFLAGS vs not.  So it really had to handle both compile and link
cases which could overlap.  Then the seperate new linker toolchain
wrapper would handle the sole linker case.  This resulted in extra
bookkeeping flags and felt heavy.
New linker wrapper example ref:
http://patchwork.ozlabs.org/patch/963748/
http://patchwork.ozlabs.org/patch/963743/

The hybrid approach updates the existing GCC wrapper to handle the
conditional fix-ups to the CFLAGS and utilizes the linker specfile
which would be included when GCC links via LDFLAGS to handle the
more basic link time fix-up.  The linker specfile would also be
included when LD is directly used and satisfy that case.  This handles
the sunny day scenario where the FLAGS are being used correctly
and doesn't try to accomodate/fix package mis-use.  It does force
the compile time fixup, however when testing I noticed it was not a
good idea to try and force the link time behavior (ie. if LDFLAGS were
not explicitly provided).  In most cases when I forced the link time
behavior, I ended up with broken builds (had an offline  autobuilder
helping me assess this).  Link time adjustments for flag usage in the
special case, ideally need to be worked as a build output is reviewed
on a case by case basis.  This also helps manage the situations where
autotools and similar build configuration/capability testing, may
conflict with flags being forced whenever GCC or LD are invoked.

Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>

---
Changes
v2 -> v3
 - Fell back on a linker approach using a spec file and kept compiler
   flag tweaks in the wrapper

v1 -> v2
 - Reworked handling of pie/pic/shared to replace each time they
   occur with a dummy string and then insert the right combination
   when rebuilding the exec string.
 - Fixed mix of tabs and spaces
 - Swapped order of shared and pie.  Originally coded it backwards.
---
 package/Makefile.in            |  4 ++--
 toolchain/gcc-specs-pie-ld     |  2 ++
 toolchain/toolchain-wrapper.c  | 20 +++++++++++++++++++-
 toolchain/toolchain-wrapper.mk |  4 ++++
 4 files changed, 27 insertions(+), 3 deletions(-)
 create mode 100644 toolchain/gcc-specs-pie-ld

On 13/09/2018 22:26, Matt Weber wrote:
> The second option was the addition of a new linker wrapper but after
> completing that approach, the amount of complexity and new software,
> seems to out weigh possibly doing a hybrid approach proposed in this
> patchset.  The one main complexity of solely using wrappers, is the
> fact that the GCC wrapper needs to know if it is being called with
> LDFLAGS vs not.  So it really had to handle both compile and link
> cases which could overlap.  Then the seperate new linker toolchain
> wrapper would handle the sole linker case.  This resulted in extra
> bookkeeping flags and felt heavy.
> New linker wrapper example ref:
> http://patchwork.ozlabs.org/patch/963748/
> http://patchwork.ozlabs.org/patch/963743/

 I still don't understand why it doesn't work with just manipulating the options
from the wrapper. I.e., apply http://patchwork.ozlabs.org/patch/963743/ but
without the linker wrapper. Do you have a concrete example of a build failure?

 As far as I can see, that approach corresponds entirely to what a normal distro
does, only it is done through the wrapper instead of through the spec file. But
in the end, the spec file has the same logic as you had in the wrapper:

%{!static:%{!shared:%{!r:-pie}}}

is equivalent to

	for (i = 1; i < argc; i++) {
		if (!strcmp(argv[i], "-r") ||
		    !strcmp(argv[i], "-static") ||
		    !strcmp(argv[i], "-shared"))
			break;
	}

	if (i == argc) {
		*cur++ = "-pie";
	}

And I think it was noted before: the spec file is not used when ld is called
directly, so the linker wrapper is NOT equivalent to using a spec file. Since
distros seem to use the spec file approach, apparently it is OK to not handle
the case when the linker is called directly.


 Regards,
 Arnout

Arnout,
On Thu, Sep 13, 2018 at 4:06 PM Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
>
> On 13/09/2018 22:26, Matt Weber wrote:
> > The second option was the addition of a new linker wrapper but after
> > completing that approach, the amount of complexity and new software,
> > seems to out weigh possibly doing a hybrid approach proposed in this
> > patchset.  The one main complexity of solely using wrappers, is the
> > fact that the GCC wrapper needs to know if it is being called with
> > LDFLAGS vs not.  So it really had to handle both compile and link
> > cases which could overlap.  Then the seperate new linker toolchain
> > wrapper would handle the sole linker case.  This resulted in extra
> > bookkeeping flags and felt heavy.
> > New linker wrapper example ref:
> > http://patchwork.ozlabs.org/patch/963748/
> > http://patchwork.ozlabs.org/patch/963743/
>
>  I still don't understand why it doesn't work with just manipulating the options
> from the wrapper. I.e., apply http://patchwork.ozlabs.org/patch/963743/ but
> without the linker wrapper. Do you have a concrete example of a build failure?

It isn't that there is a build failure.  In-order to have a elf with
PIE enabled.  The "-pie" option needs to be conditionally set only
when GCC is linking or LD is directly used (This is obviously not
perfect as packages do what they like and some need fixing to stay
within those assumptions). The example I provided in my cover letter
shows a case where the build is successful but the runtime test  (elf
inspection using checksec for PIE) fails when you remove the specfile
from the LDFLAGS.  I'll have to specifically look if it was lighttpd
or busybox that shows this behavior.  There are more packages then
that.  I can check my larger ~80 pkg test build and compare with and
without the specfile set and see the PIE enabled delta between the two
builds.  Most packages elf's end up with it disabled without the
linker specfile or equivalent wrapper used to set -pie.

>
>  As far as I can see, that approach corresponds entirely to what a normal distro
> does, only it is done through the wrapper instead of through the spec file. But
> in the end, the spec file has the same logic as you had in the wrapper:
>
> %{!static:%{!shared:%{!r:-pie}}}
>
> is equivalent to
>
>         for (i = 1; i < argc; i++) {
>                 if (!strcmp(argv[i], "-r") ||
>                     !strcmp(argv[i], "-static") ||
>                     !strcmp(argv[i], "-shared"))
>                         break;
>         }
>
>         if (i == argc) {
>                 *cur++ = "-pie";
>         }
>

Correct, which is why in my latest patchset, I reverted back to using
the specfile for the linker logic as it didn't seem to make sense to
create all the linker wrapper stuff just for this case.  Plus there is
complexity with needing to make the GCC wrapper link aware so it can
also conditionally set -pie when GCC is linking (I used a flag in my
LDFLAGS and used that to trigger doing to logic for adding -pie).

> And I think it was noted before: the spec file is not used when ld is called
> directly, so the linker wrapper is NOT equivalent to using a spec file. Since
> distros seem to use the spec file approach, apparently it is OK to not handle
> the case when the linker is called directly.

I had assumed the specfile is used when LD is called directly.  Humm,
I'll have to think about that case.  This current patchset shouldn't
need any adjusting, just more of a proof to convince ourself that the
specfile isn't going anything when the LD is directly called.  I
believe I had cases where busybox, valgrind and boost where using LD
directly and we were having to manually adjust the -shared and -pie
order prior to having the link wrapper/specfile.


Matt

Arnout,
On Thu, Sep 13, 2018 at 7:20 PM Matthew Weber
<matthew.weber@rockwellcollins.com> wrote:
>
> Arnout,
> On Thu, Sep 13, 2018 at 4:06 PM Arnout Vandecappelle <arnout@mind.be> wrote:
> >
> >
> >
> > On 13/09/2018 22:26, Matt Weber wrote:
> > > The second option was the addition of a new linker wrapper but after
> > > completing that approach, the amount of complexity and new software,
> > > seems to out weigh possibly doing a hybrid approach proposed in this
> > > patchset.  The one main complexity of solely using wrappers, is the
> > > fact that the GCC wrapper needs to know if it is being called with
> > > LDFLAGS vs not.  So it really had to handle both compile and link
> > > cases which could overlap.  Then the seperate new linker toolchain
> > > wrapper would handle the sole linker case.  This resulted in extra
> > > bookkeeping flags and felt heavy.
> > > New linker wrapper example ref:
> > > http://patchwork.ozlabs.org/patch/963748/
> > > http://patchwork.ozlabs.org/patch/963743/
> >
> >  I still don't understand why it doesn't work with just manipulating the options
> > from the wrapper. I.e., apply http://patchwork.ozlabs.org/patch/963743/ but
> > without the linker wrapper. Do you have a concrete example of a build failure?
>

I've gone back and retested this specific configuration.  I found out
that the wrapper debug actually causes failures.  I was able to
confirm with wrapper debug disabled that the compile wrapper with the
logic of the linker specfile works.   There is no link time LD tool
dependency or use of that specfile.  I didn't realize the specfile was
only used by the gcc frontend.  I'll respin a patchset today with this
fixup.  I believe there are a few tools that will need us to add
no-pie but I assume that can be a follow-up patchset.  (Paxtest is one
of those tools)

Matt

On 14/09/2018 02:20, Matthew Weber wrote:
> Arnout,
> On Thu, Sep 13, 2018 at 4:06 PM Arnout Vandecappelle <arnout@mind.be> wrote:
[snip]
>> And I think it was noted before: the spec file is not used when ld is called
>> directly, so the linker wrapper is NOT equivalent to using a spec file. Since
>> distros seem to use the spec file approach, apparently it is OK to not handle
>> the case when the linker is called directly.
> 
> I had assumed the specfile is used when LD is called directly.  Humm,
> I'll have to think about that case.  This current patchset shouldn't
> need any adjusting, just more of a proof to convince ourself that the
> specfile isn't going anything when the LD is directly called.  I
> believe I had cases where busybox, valgrind and boost where using LD
> directly and we were having to manually adjust the -shared and -pie
> order prior to having the link wrapper/specfile.

 ld is normally called directly in only three situations:

* when creating a shared library; in that case, we shouldn't add -pie anyway;

* when creating a freestanding executable (i.e., a bootloader or kernel); in
that case, it's very unlikely that adding -pie would be a good idea;

* for Go and Rust, because they don't use crt0 and crtend but have their own
(they might have their own LD as well, I'm not sure); for these, because the
code itself is not built with gcc, they wouldn't get the -fPIE flag so linking
with -pie would probably fail.

 Bottom line: I can't think of a situation where we need to do something when
the package calls LD directly. Of course, this is theory :-)

 Regards,
 Arnout

Arnout,
On Fri, Sep 14, 2018 at 3:32 PM Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
>
> On 14/09/2018 02:20, Matthew Weber wrote:
> > Arnout,
> > On Thu, Sep 13, 2018 at 4:06 PM Arnout Vandecappelle <arnout@mind.be> wrote:
> [snip]
> >> And I think it was noted before: the spec file is not used when ld is called
> >> directly, so the linker wrapper is NOT equivalent to using a spec file. Since
> >> distros seem to use the spec file approach, apparently it is OK to not handle
> >> the case when the linker is called directly.
> >
> > I had assumed the specfile is used when LD is called directly.  Humm,
> > I'll have to think about that case.  This current patchset shouldn't
> > need any adjusting, just more of a proof to convince ourself that the
> > specfile isn't going anything when the LD is directly called.  I
> > believe I had cases where busybox, valgrind and boost where using LD
> > directly and we were having to manually adjust the -shared and -pie
> > order prior to having the link wrapper/specfile.
>
>  ld is normally called directly in only three situations:
>
> * when creating a shared library; in that case, we shouldn't add -pie anyway;
>
> * when creating a freestanding executable (i.e., a bootloader or kernel); in
> that case, it's very unlikely that adding -pie would be a good idea;
>
> * for Go and Rust, because they don't use crt0 and crtend but have their own
> (they might have their own LD as well, I'm not sure); for these, because the
> code itself is not built with gcc, they wouldn't get the -fPIE flag so linking
> with -pie would probably fail.

I'm assuming there will be cases where we need to go in and set
-fno-pie.  I have not ran into that yet with my test targets.  We'll
see if my offline autobuilder finds them.

>
>  Bottom line: I can't think of a situation where we need to do something when
> the package calls LD directly. Of course, this is theory :-)
>

Completely agree, I didn't understand this assumption until yesterday
:-)  Latest patchset should reflect this.

Matt