[5/5,v2] toolchain: allow PIC/PIE without RELRO

Message ID 16391_1552392582_5C87A186_16391_281_1_704ea354-404e-4e6b-af91-9e63897e78c9@OPEXCLILM6F.corporate.adroot.infra.ftgroup
State New
Headers show
Series
  • [1/5,v2] toolchain: prepare to pass more additional CFLAGS via the wrapper
Related show

Commit Message

Yann E. MORIN March 12, 2019, 12:09 p.m.
From: "Yann E. MORIN" <yann.morin@orange.com>

In commit 7484c1c3b806 (toolchain/toolchain-wrapper: add BR2_RELRO_),
we added the PIC/PIE flags, but based on the RELRO_FULL condition.

It is however totally possible to do a PIC/PIE executable without
RELRO_FULL, as it is also valid to do a PIC/PIE build with RELRO_PARTIAL.

Add a new option that now governs the PIC/PIE flags.

Note: it is unknown if RELRO_FULL really needs PIC/PIE or not, so we
keep the current situation, where RELRO-FULL forces PIC/PIE compilation.
Decoupling can come later from an interested party.

Signed-off-by: "Yann E. MORIN" <yann.morin@orange.com>
Cc: Matt Weber <matthew.weber@rockwellcollins.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 Config.in                      | 8 ++++++++
 toolchain/toolchain-wrapper.c  | 2 +-
 toolchain/toolchain-wrapper.mk | 4 ++++
 3 files changed, 13 insertions(+), 1 deletion(-)

Comments

Matthew Weber March 12, 2019, 1:50 p.m. | #1
Yann,
On Tue, Mar 12, 2019 at 7:09 AM <yann.morin@orange.com> wrote:
>
> From: "Yann E. MORIN" <yann.morin@orange.com>
>
> In commit 7484c1c3b806 (toolchain/toolchain-wrapper: add BR2_RELRO_),
> we added the PIC/PIE flags, but based on the RELRO_FULL condition.
>
> It is however totally possible to do a PIC/PIE executable without
> RELRO_FULL, as it is also valid to do a PIC/PIE build with RELRO_PARTIAL.
>
> Add a new option that now governs the PIC/PIE flags.
>
> Note: it is unknown if RELRO_FULL really needs PIC/PIE or not, so we
> keep the current situation, where RELRO-FULL forces PIC/PIE compilation.
> Decoupling can come later from an interested party.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin@orange.com>
> Cc: Matt Weber <matthew.weber@rockwellcollins.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> ---
>  Config.in                      | 8 ++++++++
>  toolchain/toolchain-wrapper.c  | 2 +-
>  toolchain/toolchain-wrapper.mk | 4 ++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/Config.in b/Config.in
> index d5a0460f98..31fea3ab34 100644
> --- a/Config.in
> +++ b/Config.in
> @@ -712,6 +712,13 @@ endmenu
>
>  comment "Security Hardening Options"
>
> +config BR2_PIC_PIE
> +       bool "Build code with PIC/PIE"
> +       depends on BR2_SHARED_LIBS
> +       help
> +         Generate Position-Independent Code (PIC) and link
> +         Position-Independent Executables (PIE).
> +
>  choice
>         bool "Stack Smashing Protection"
>         default BR2_SSP_ALL if BR2_ENABLE_SSP # legacy
> @@ -794,6 +801,7 @@ config BR2_RELRO_PARTIAL
>
>  config BR2_RELRO_FULL
>         bool "Full"
> +       select BR2_PIC_PIE

In the previous email chain it was being discussed if PIC/PIE was
required for full RELRO.  Like you guys mentioned, I believe it was
always just lumped into the configuration the other distros called
"full".  However, I'm pretty sure they are independent and you could
have full RELRO without PIC/PIE  (I did not test this theory, just
checked some docs and the theory holds).  I'd be on the fence if we
should remove this select and keep the BR2_PIC_PIE as something you'd
need to independently set if you want it with BR2_RELRO_FULL.  I
guess, better to make that sort of change now then after more time has
gone by.

>         help
>           This option includes the partial configuration, but also marks
>           the GOT as read-only at the cost of initialization time during
> diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
> index c73a0cc079..7a4b9c4007 100644
> --- a/toolchain/toolchain-wrapper.c
> +++ b/toolchain/toolchain-wrapper.c
> @@ -367,7 +367,7 @@ int main(int argc, char **argv)
>                 *cur++ = "-Wno-builtin-macro-redefined";
>         }
>
> -#ifdef BR2_RELRO_FULL
> +#ifdef BR2_PIC_PIE

Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>
Yann E. MORIN March 12, 2019, 2:25 p.m. | #2
Matt, All,

On 2019-03-12 08:50 -0500, Matthew Weber spake thusly:
> On Tue, Mar 12, 2019 at 7:09 AM <yann.morin@orange.com> wrote:
> > From: "Yann E. MORIN" <yann.morin@orange.com>
> > In commit 7484c1c3b806 (toolchain/toolchain-wrapper: add BR2_RELRO_),
> > we added the PIC/PIE flags, but based on the RELRO_FULL condition.
[--SNIP--]
> >  config BR2_RELRO_FULL
> >         bool "Full"
> > +       select BR2_PIC_PIE
> 
> In the previous email chain it was being discussed if PIC/PIE was
> required for full RELRO.  Like you guys mentioned, I believe it was
> always just lumped into the configuration the other distros called
> "full".  However, I'm pretty sure they are independent and you could
> have full RELRO without PIC/PIE  (I did not test this theory, just
> checked some docs and the theory holds).

Arnout did test it, and it indeed works.

>  I'd be on the fence if we
> should remove this select and keep the BR2_PIC_PIE as something you'd
> need to independently set if you want it with BR2_RELRO_FULL.  I
> guess, better to make that sort of change now then after more time has
> gone by.

I did not do that in this patch, to introduce the minimal disruption
possible. I.e. a configuration which had relro-full will still get
PIC/PIE. This patch just adds the possiblity to do PIC/PIE without
relro-full.

If we really want to decouple the two, then I think we should do that in
a separate patch, which just drops this new select, to allow relro-full
without PIC/PIE.

I'll do that in a followup patch.

Regards,
Yann E. MORIN.

> >         help
> >           This option includes the partial configuration, but also marks
> >           the GOT as read-only at the cost of initialization time during
> > diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
> > index c73a0cc079..7a4b9c4007 100644
> > --- a/toolchain/toolchain-wrapper.c
> > +++ b/toolchain/toolchain-wrapper.c
> > @@ -367,7 +367,7 @@ int main(int argc, char **argv)
> >                 *cur++ = "-Wno-builtin-macro-redefined";
> >         }
> >
> > -#ifdef BR2_RELRO_FULL
> > +#ifdef BR2_PIC_PIE
> 
> Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

Patch

diff --git a/Config.in b/Config.in
index d5a0460f98..31fea3ab34 100644
--- a/Config.in
+++ b/Config.in
@@ -712,6 +712,13 @@  endmenu
 
 comment "Security Hardening Options"
 
+config BR2_PIC_PIE
+	bool "Build code with PIC/PIE"
+	depends on BR2_SHARED_LIBS
+	help
+	  Generate Position-Independent Code (PIC) and link
+	  Position-Independent Executables (PIE).
+
 choice
 	bool "Stack Smashing Protection"
 	default BR2_SSP_ALL if BR2_ENABLE_SSP # legacy
@@ -794,6 +801,7 @@  config BR2_RELRO_PARTIAL
 
 config BR2_RELRO_FULL
 	bool "Full"
+	select BR2_PIC_PIE
 	help
 	  This option includes the partial configuration, but also marks
 	  the GOT as read-only at the cost of initialization time during
diff --git a/toolchain/toolchain-wrapper.c b/toolchain/toolchain-wrapper.c
index c73a0cc079..7a4b9c4007 100644
--- a/toolchain/toolchain-wrapper.c
+++ b/toolchain/toolchain-wrapper.c
@@ -367,7 +367,7 @@  int main(int argc, char **argv)
 		*cur++ = "-Wno-builtin-macro-redefined";
 	}
 
-#ifdef BR2_RELRO_FULL
+#ifdef BR2_PIC_PIE
 	/* Patterned after Fedora/Gentoo hardening approaches.
 	 * https://fedoraproject.org/wiki/Changes/Harden_All_Packages
 	 * https://wiki.gentoo.org/wiki/Hardened/Toolchain#Position_Independent_Executables_.28PIEs.29
diff --git a/toolchain/toolchain-wrapper.mk b/toolchain/toolchain-wrapper.mk
index ca66fa7ba4..3c42146cea 100644
--- a/toolchain/toolchain-wrapper.mk
+++ b/toolchain/toolchain-wrapper.mk
@@ -48,6 +48,10 @@  ifeq ($(BR2_CCACHE_USE_BASEDIR),y)
 TOOLCHAIN_WRAPPER_ARGS += -DBR_CCACHE_BASEDIR='"$(BASE_DIR)"'
 endif
 
+ifeq ($(BR2_PIC_PIE),y)
+TOOLCHAIN_WRAPPER_ARGS += -DBR2_PIC_PIE
+endif
+
 ifeq ($(BR2_RELRO_PARTIAL),y)
 TOOLCHAIN_WRAPPER_ARGS += -DBR2_RELRO_PARTIAL
 else ifeq ($(BR2_RELRO_FULL),y)