diff mbox

gcc: add option to build and install gcc's libssp

Message ID AM3PR05MB433082E20B48215DFA88130C9CD0@AM3PR05MB433.eurprd05.prod.outlook.com
State Rejected
Headers show

Commit Message

Guy Benyei June 12, 2017, 8:23 a.m. UTC
When stack smashing protection is not provided by the C library, libssp
provides the functionality needed to support the -fstack-protector flag.

Signed-off-by: Guy Benyei <guybe@mellanox.com>
---
 package/gcc/Config.in.host         |   14 ++++++++++++++
 package/gcc/gcc-final/gcc-final.mk |   11 +++++++++++
 package/gcc/gcc.mk                 |    8 ++++++--
 package/glibc/Config.in            |    1 +
 package/musl/Config.in             |    3 ++-
 package/uclibc/Config.in           |    1 +
 toolchain/toolchain-common.in      |    3 +++
 7 files changed, 38 insertions(+), 3 deletions(-)

Comments

Arnout Vandecappelle June 12, 2017, 10:08 p.m. UTC | #1
Hi Guy,

On 12-06-17 10:23, Guy Benyei wrote:
> When stack smashing protection is not provided by the C library, libssp
> provides the functionality needed to support the -fstack-protector flag.

 I have a few details that can be improved below. However, I still wonder
whether this feature is useful at all.

 The new symbol will only appear in two cases:

- with uClibc, if BR2_TOOLCHAIN_BUILDROOT_USE_SSP is not selected;
- with musl, for i386 and powerpc (because it is broken on musl).

 In the uClibc case, I really see no reason to prefer the gcc version instead of
the uClibc version. So that basically leaves just the musl case. Is that the
situation you are trying to fix? If that is it, I think it's better not to have
a config options at all, but just enable the gcc thing if musl && (i386 || powerpc).

 Or is there another reason why you need this?

[snip]
> +config BR2_TOOLCHAIN_INSTALL_LIBSSP
> +	bool "Enable stack protection support using libssp"
> +	depends on !BR2_LIBC_HAS_SSP
> +	select BR2_TOOLCHAIN_HAS_SSP
> +	help
> +	  Enable stack smashing protection support using GCCs
> +	  -fstack-protector option by installing libssp. This
> +	  option should be used only when the C library doesn't
> +	  support stack smashing protection.

 Since now this option only appears when the C library doesn't support ssp, this
second sentence is redundant.

> +
> +	  See http://wiki.osdev.org/Stack_Smashing_Protector#libssp
> +	  for details.
> +
> diff --git a/package/gcc/gcc-final/gcc-final.mk b/package/gcc/gcc-final/gcc-final.mk
> index e8d2e18..67106ed 100644
> --- a/package/gcc/gcc-final/gcc-final.mk
> +++ b/package/gcc/gcc-final/gcc-final.mk
> @@ -147,6 +147,17 @@ endef
>  
>  HOST_GCC_FINAL_POST_INSTALL_HOOKS += HOST_GCC_FINAL_INSTALL_LIBATOMIC
>  
> +define HOST_GCC_FINAL_INSTALL_LIBSSP
> +	-cp -dpf $(HOST_GCC_FINAL_GCC_LIB_DIR)/libssp* \
> +		$(STAGING_DIR)/lib/
> +	-cp -dpf $(HOST_GCC_FINAL_GCC_LIB_DIR)/libssp* \
> +		$(TARGET_DIR)/lib/
> +endef

 This is the third post-install-hook that is used only to install in /lib, so
perhaps it's time to refactor that into HOST_GCC_FINAL_LIBS and a loop like for
/usr/lib. But that can be done in a follow-up patch.

> +
> +ifeq ($(BR2_TOOLCHAIN_INSTALL_LIBSSP),y)
> +HOST_GCC_FINAL_POST_INSTALL_HOOKS += HOST_GCC_FINAL_INSTALL_LIBSSP
> +endif
> +
>  # Handle the installation of libraries in /usr/lib
>  HOST_GCC_FINAL_USR_LIBS =
>  
> diff --git a/package/gcc/gcc.mk b/package/gcc/gcc.mk
> index b52f945..4691846 100644
> --- a/package/gcc/gcc.mk
> +++ b/package/gcc/gcc.mk
> @@ -90,7 +90,6 @@ HOST_GCC_COMMON_CONF_OPTS = \
>  	--with-sysroot=$(STAGING_DIR) \
>  	--disable-__cxa_atexit \
>  	--with-gnu-ld \
> -	--disable-libssp \
>  	--disable-multilib \
>  	--with-gmp=$(HOST_DIR)/usr \
>  	--with-mpc=$(HOST_DIR)/usr \
> @@ -110,6 +109,11 @@ GCC_COMMON_TARGET_CXXFLAGS = $(TARGET_CXXFLAGS)
>  HOST_GCC_COMMON_CONF_ENV += CFLAGS_FOR_TARGET="$(GCC_COMMON_TARGET_CFLAGS)"
>  HOST_GCC_COMMON_CONF_ENV += CXXFLAGS_FOR_TARGET="$(GCC_COMMON_TARGET_CXXFLAGS)"
>  
> +# don't build libssp if it's not explicitly required
> +ifneq ($(BR2_TOOLCHAIN_INSTALL_LIBSSP),y)
> +HOST_GCC_COMMON_CONF_OPTS += --disable-libssp

 Please also add an explicit --enable. That also allows you to use positive
logic for the condition.

> +endif
> +
>  # libitm needs sparc V9+
>  ifeq ($(BR2_sparc_v8)$(BR2_sparc_leon3),y)
>  HOST_GCC_COMMON_CONF_OPTS += --disable-libitm
> @@ -286,7 +290,7 @@ endif # !BR2_GCC_ARCH_HAS_CONFIGURABLE_DEFAULTS
>  # available or not in the C library is not working properly for
>  # uClibc, so let's be explicit as well.
>  HOST_GCC_COMMON_MAKE_OPTS = \
> -	gcc_cv_libc_provides_ssp=$(if $(BR2_TOOLCHAIN_HAS_SSP),yes,no)
> +	gcc_cv_libc_provides_ssp=$(if $(BR2_LIBC_HAS_SSP),yes,no)
>  
>  ifeq ($(BR2_CCACHE),y)
>  HOST_GCC_COMMON_CCACHE_HASH_FILES += $(DL_DIR)/$(GCC_SOURCE)
> diff --git a/package/glibc/Config.in b/package/glibc/Config.in
> index 115388e..d5494b7 100644
> --- a/package/glibc/Config.in
> +++ b/package/glibc/Config.in
> @@ -4,6 +4,7 @@ config BR2_PACKAGE_GLIBC
>  	bool
>  	default y
>  	select BR2_PACKAGE_LINUX_HEADERS
> +	select BR2_LIBC_HAS_SSP
>  	select BR2_TOOLCHAIN_HAS_SSP

 Instead of having both here, move the "select BR2_TOOLCHAIN_HAS_SSP" to the
definition of BR2_LIBC_HAS_SSP.

>  
>  choice
> diff --git a/package/musl/Config.in b/package/musl/Config.in
> index bedc50c..6b3d542 100644
> --- a/package/musl/Config.in
> +++ b/package/musl/Config.in
> @@ -4,6 +4,7 @@ config BR2_PACKAGE_MUSL
>  	depends on BR2_TOOLCHAIN_USES_MUSL
>  	select BR2_PACKAGE_LINUX_HEADERS
>  	# SSP broken on i386/ppc: http://www.openwall.com/lists/musl/2016/12/04/2
> -	select BR2_TOOLCHAIN_HAS_SSP if !(BR2_i386 || BR2_powerpc)
> +	select BR2_LIBC_HAS_SSP if !(BR2_i386 || BR2_powerpc)
> +	select BR2_TOOLCHAIN_HAS_SSP if BR2_LIBC_HAS_SSP
>  	# Compatibility headers: cdefs.h, queue.h
>  	select BR2_PACKAGE_MUSL_COMPAT_HEADERS
> diff --git a/package/uclibc/Config.in b/package/uclibc/Config.in
> index b0b0b01..b87125c 100644
> --- a/package/uclibc/Config.in
> +++ b/package/uclibc/Config.in
> @@ -71,6 +71,7 @@ config BR2_PTHREAD_DEBUG
>  
>  config BR2_TOOLCHAIN_BUILDROOT_USE_SSP
>  	bool "Enable stack protection support"
> +	select BR2_LIBC_HAS_SSP
>  	select BR2_TOOLCHAIN_HAS_SSP
>  	help
>  	  Enable stack smashing protection support using GCCs
> diff --git a/toolchain/toolchain-common.in b/toolchain/toolchain-common.in
> index d670f44..3ffb5fb 100644
> --- a/toolchain/toolchain-common.in
> +++ b/toolchain/toolchain-common.in
> @@ -45,6 +45,9 @@ config BR2_TOOLCHAIN_HAS_THREADS_NPTL
>  config BR2_TOOLCHAIN_HAS_SHADOW_PASSWORDS
>  	bool
>  
> +config BR2_LIBC_HAS_SSP
> +	bool

 i.e. here.

 Regards,
 Arnout

> +
>  config BR2_TOOLCHAIN_HAS_SSP
>  	bool
>  
>
Thomas Petazzoni June 13, 2017, 7:08 a.m. UTC | #2
Hello,

On Tue, 13 Jun 2017 00:08:35 +0200, Arnout Vandecappelle wrote:

> On 12-06-17 10:23, Guy Benyei wrote:
> > When stack smashing protection is not provided by the C library, libssp
> > provides the functionality needed to support the -fstack-protector flag.  
> 
>  I have a few details that can be improved below. However, I still wonder
> whether this feature is useful at all.
> 
>  The new symbol will only appear in two cases:
> 
> - with uClibc, if BR2_TOOLCHAIN_BUILDROOT_USE_SSP is not selected;
> - with musl, for i386 and powerpc (because it is broken on musl).
> 
>  In the uClibc case, I really see no reason to prefer the gcc version instead of
> the uClibc version. So that basically leaves just the musl case. Is that the
> situation you are trying to fix? If that is it, I think it's better not to have
> a config options at all, but just enable the gcc thing if musl && (i386 || powerpc).
> 
>  Or is there another reason why you need this?

Yes, I'm also wondering about the use case here. Is there any benefit
in using the compiler-provided libssp, instead of the libc provided
implementation? Or, opposite question: is there any benefit in using
the libc provided implementation (as we do today) instead of the
compiler-provided one?

Should we provide a choice for the user to select between the two? I
must say I'm not really thrilled by this idea because it's a very
arcane/specific configuration item, which may be difficult for
regular users to understand.

Best regards,

Thomas
Guy Benyei June 13, 2017, 7:40 a.m. UTC | #3
Hello,

On Tuesday, June 13, 2017 10:09 AM, Thomas Petazzoni wrote:


> >  The new symbol will only appear in two cases:
> >
> > - with uClibc, if BR2_TOOLCHAIN_BUILDROOT_USE_SSP is not selected;
> > - with musl, for i386 and powerpc (because it is broken on musl).
> >
> >  In the uClibc case, I really see no reason to prefer the gcc version
> > instead of the uClibc version. So that basically leaves just the musl
> > case. Is that the situation you are trying to fix? If that is it, I
> > think it's better not to have a config options at all, but just enable the gcc thing
> if musl && (i386 || powerpc).
> >
> >  Or is there another reason why you need this?
> 
> Yes, I'm also wondering about the use case here. Is there any benefit in using the
> compiler-provided libssp, instead of the libc provided implementation? Or,
> opposite question: is there any benefit in using the libc provided implementation
> (as we do today) instead of the compiler-provided one?
> 
> Should we provide a choice for the user to select between the two? I must say
> I'm not really thrilled by this idea because it's a very arcane/specific configuration
> item, which may be difficult for regular users to understand.

I'm actually using uClibc on ARC big-endian architecture, so my target is not musl. We also don't want to enable SSP all the time, only as a tool when memory issues are being debugged. My goal was to provide SSP without any change to uClibc or GCC itself. LibSSP could be copied to the target only when SSP is actually needed.

When double checking the result of compiling uClibc with BR2_TOOLCHAIN_BUILDROOT_USE_SSP, I've seen, that uClibc is actually binary-identical to the version built without BR2_TOOLCHAIN_BUILDROOT_USE_SSP. I find it strange, as actually there should be some additional functions and variables enabled in this case. Anyhow,  might be able to work with the uClibc support enabled by default.

Thanks
      Guy
diff mbox

Patch

diff --git a/package/gcc/Config.in.host b/package/gcc/Config.in.host
index 5dcaa03..a89af6a 100644
--- a/package/gcc/Config.in.host
+++ b/package/gcc/Config.in.host
@@ -159,3 +159,17 @@  config BR2_GCC_ENABLE_GRAPHITE
 	help
 	  This option enables the graphite optimizations in the
 	  compiler.
+
+config BR2_TOOLCHAIN_INSTALL_LIBSSP
+	bool "Enable stack protection support using libssp"
+	depends on !BR2_LIBC_HAS_SSP
+	select BR2_TOOLCHAIN_HAS_SSP
+	help
+	  Enable stack smashing protection support using GCCs
+	  -fstack-protector option by installing libssp. This
+	  option should be used only when the C library doesn't
+	  support stack smashing protection.
+
+	  See http://wiki.osdev.org/Stack_Smashing_Protector#libssp
+	  for details.
+
diff --git a/package/gcc/gcc-final/gcc-final.mk b/package/gcc/gcc-final/gcc-final.mk
index e8d2e18..67106ed 100644
--- a/package/gcc/gcc-final/gcc-final.mk
+++ b/package/gcc/gcc-final/gcc-final.mk
@@ -147,6 +147,17 @@  endef
 
 HOST_GCC_FINAL_POST_INSTALL_HOOKS += HOST_GCC_FINAL_INSTALL_LIBATOMIC
 
+define HOST_GCC_FINAL_INSTALL_LIBSSP
+	-cp -dpf $(HOST_GCC_FINAL_GCC_LIB_DIR)/libssp* \
+		$(STAGING_DIR)/lib/
+	-cp -dpf $(HOST_GCC_FINAL_GCC_LIB_DIR)/libssp* \
+		$(TARGET_DIR)/lib/
+endef
+
+ifeq ($(BR2_TOOLCHAIN_INSTALL_LIBSSP),y)
+HOST_GCC_FINAL_POST_INSTALL_HOOKS += HOST_GCC_FINAL_INSTALL_LIBSSP
+endif
+
 # Handle the installation of libraries in /usr/lib
 HOST_GCC_FINAL_USR_LIBS =
 
diff --git a/package/gcc/gcc.mk b/package/gcc/gcc.mk
index b52f945..4691846 100644
--- a/package/gcc/gcc.mk
+++ b/package/gcc/gcc.mk
@@ -90,7 +90,6 @@  HOST_GCC_COMMON_CONF_OPTS = \
 	--with-sysroot=$(STAGING_DIR) \
 	--disable-__cxa_atexit \
 	--with-gnu-ld \
-	--disable-libssp \
 	--disable-multilib \
 	--with-gmp=$(HOST_DIR)/usr \
 	--with-mpc=$(HOST_DIR)/usr \
@@ -110,6 +109,11 @@  GCC_COMMON_TARGET_CXXFLAGS = $(TARGET_CXXFLAGS)
 HOST_GCC_COMMON_CONF_ENV += CFLAGS_FOR_TARGET="$(GCC_COMMON_TARGET_CFLAGS)"
 HOST_GCC_COMMON_CONF_ENV += CXXFLAGS_FOR_TARGET="$(GCC_COMMON_TARGET_CXXFLAGS)"
 
+# don't build libssp if it's not explicitly required
+ifneq ($(BR2_TOOLCHAIN_INSTALL_LIBSSP),y)
+HOST_GCC_COMMON_CONF_OPTS += --disable-libssp
+endif
+
 # libitm needs sparc V9+
 ifeq ($(BR2_sparc_v8)$(BR2_sparc_leon3),y)
 HOST_GCC_COMMON_CONF_OPTS += --disable-libitm
@@ -286,7 +290,7 @@  endif # !BR2_GCC_ARCH_HAS_CONFIGURABLE_DEFAULTS
 # available or not in the C library is not working properly for
 # uClibc, so let's be explicit as well.
 HOST_GCC_COMMON_MAKE_OPTS = \
-	gcc_cv_libc_provides_ssp=$(if $(BR2_TOOLCHAIN_HAS_SSP),yes,no)
+	gcc_cv_libc_provides_ssp=$(if $(BR2_LIBC_HAS_SSP),yes,no)
 
 ifeq ($(BR2_CCACHE),y)
 HOST_GCC_COMMON_CCACHE_HASH_FILES += $(DL_DIR)/$(GCC_SOURCE)
diff --git a/package/glibc/Config.in b/package/glibc/Config.in
index 115388e..d5494b7 100644
--- a/package/glibc/Config.in
+++ b/package/glibc/Config.in
@@ -4,6 +4,7 @@  config BR2_PACKAGE_GLIBC
 	bool
 	default y
 	select BR2_PACKAGE_LINUX_HEADERS
+	select BR2_LIBC_HAS_SSP
 	select BR2_TOOLCHAIN_HAS_SSP
 
 choice
diff --git a/package/musl/Config.in b/package/musl/Config.in
index bedc50c..6b3d542 100644
--- a/package/musl/Config.in
+++ b/package/musl/Config.in
@@ -4,6 +4,7 @@  config BR2_PACKAGE_MUSL
 	depends on BR2_TOOLCHAIN_USES_MUSL
 	select BR2_PACKAGE_LINUX_HEADERS
 	# SSP broken on i386/ppc: http://www.openwall.com/lists/musl/2016/12/04/2
-	select BR2_TOOLCHAIN_HAS_SSP if !(BR2_i386 || BR2_powerpc)
+	select BR2_LIBC_HAS_SSP if !(BR2_i386 || BR2_powerpc)
+	select BR2_TOOLCHAIN_HAS_SSP if BR2_LIBC_HAS_SSP
 	# Compatibility headers: cdefs.h, queue.h
 	select BR2_PACKAGE_MUSL_COMPAT_HEADERS
diff --git a/package/uclibc/Config.in b/package/uclibc/Config.in
index b0b0b01..b87125c 100644
--- a/package/uclibc/Config.in
+++ b/package/uclibc/Config.in
@@ -71,6 +71,7 @@  config BR2_PTHREAD_DEBUG
 
 config BR2_TOOLCHAIN_BUILDROOT_USE_SSP
 	bool "Enable stack protection support"
+	select BR2_LIBC_HAS_SSP
 	select BR2_TOOLCHAIN_HAS_SSP
 	help
 	  Enable stack smashing protection support using GCCs
diff --git a/toolchain/toolchain-common.in b/toolchain/toolchain-common.in
index d670f44..3ffb5fb 100644
--- a/toolchain/toolchain-common.in
+++ b/toolchain/toolchain-common.in
@@ -45,6 +45,9 @@  config BR2_TOOLCHAIN_HAS_THREADS_NPTL
 config BR2_TOOLCHAIN_HAS_SHADOW_PASSWORDS
 	bool
 
+config BR2_LIBC_HAS_SSP
+	bool
+
 config BR2_TOOLCHAIN_HAS_SSP
 	bool