[Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Paolo Bonzini]

From: Steven Noonan <steven@uplinklabs.net>

The -fstack-protector flag family is useful for ensuring safety and for
debugging, but has a performance impact. Here are some boot time comparisons of
the various versions of -fstack-protector using qemu-system-arm on an x86_64
host:

    # -fstack-protector-all
    Startup finished in 1.810s (kernel) + 12.331s (initrd) + 49.016s (userspace) = 1min 3.159s
    Startup finished in 1.801s (kernel) + 12.287s (initrd) + 47.925s (userspace) = 1min 2.013s
    Startup finished in 1.812s (kernel) + 12.302s (initrd) + 47.995s (userspace) = 1min 2.111s

    # -fstack-protector-strong
    Startup finished in 1.744s (kernel) + 11.223s (initrd) + 44.688s (userspace) = 57.657s
    Startup finished in 1.721s (kernel) + 11.222s (initrd) + 44.194s (userspace) = 57.138s
    Startup finished in 1.693s (kernel) + 11.250s (initrd) + 44.426s (userspace) = 57.370s

    # -fstack-protector
    Startup finished in 1.705s (kernel) + 11.409s (initrd) + 43.563s (userspace) = 56.677s
    Startup finished in 1.877s (kernel) + 11.137s (initrd) + 43.719s (userspace) = 56.734s
    Startup finished in 1.708s (kernel) + 11.141s (initrd) + 43.628s (userspace) = 56.478s

    # no stack protector
    Startup finished in 1.743s (kernel) + 11.190s (initrd) + 43.709s (userspace) = 56.643s
    Startup finished in 1.763s (kernel) + 11.216s (initrd) + 43.767s (userspace) = 56.747s
    Startup finished in 1.711s (kernel) + 11.283s (initrd) + 43.878s (userspace) = 56.873s

This patch introduces a configure option to disable the stack protector
entirely, and conditional stack protector flag selection (in order,
based on availability): -fstack-protector-strong, -fstack-protector-all,
no stack protector.

Signed-off-by: Steven Noonan <snoonan@amazon.com>
Cc: Anthony Liguori <aliguori@amazon.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
[Prefer -fstack-protector-all to -fstack-protector, suggested by
 Laurent Desnogues. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 configure | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index fb3bd05..eb0e7bb 100755
--- a/configure
+++ b/configure
@@ -198,6 +198,7 @@ audio_win_int=""
 cc_i386=i386-pc-linux-gnu-gcc
 libs_qga=""
 debug_info="yes"
+stack_protector=""
 
 # Don't accept a target_list environment variable.
 unset target_list
@@ -950,6 +951,10 @@ for opt do
   ;;
   --disable-werror) werror="no"
   ;;
+  --enable-stack-protector) stack_protector="yes"
+  ;;
+  --disable-stack-protector) stack_protector="no"
+  ;;
   --disable-curses) curses="no"
   ;;
   --enable-curses) curses="yes"
@@ -1219,6 +1224,7 @@ Advanced options (experts only):
   --disable-sparse         disable sparse checker (default)
   --disable-strip          disable stripping binaries
   --disable-werror         disable compilation abort on warning
+  --disable-stack-protector disable compiler-provided stack protection
   --disable-sdl            disable SDL
   --enable-sdl             enable SDL
   --with-sdlabi            select preferred SDL ABI 1.2 or 2.0
@@ -1439,9 +1445,15 @@ for flag in $gcc_flags; do
     fi
 done
 
-if compile_prog "-Werror -fstack-protector-all" "" ; then
-    QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
-    LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
+if test "$stack_protector" != "no" ; then
+  gcc_flags="-fstack-protector-strong -fstack-protector-all"
+  for flag in $gcc_flags; do
+    if compile_prog "-Werror $flag" "" ; then
+      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
+      break
+    fi
+  done
 fi
 
 # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Laurent Desnogues]

On Fri, Mar 28, 2014 at 5:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Steven Noonan <steven@uplinklabs.net>
>
> The -fstack-protector flag family is useful for ensuring safety and for
> debugging, but has a performance impact. Here are some boot time comparisons of
> the various versions of -fstack-protector using qemu-system-arm on an x86_64
> host:
>
>     # -fstack-protector-all
>     Startup finished in 1.810s (kernel) + 12.331s (initrd) + 49.016s (userspace) = 1min 3.159s
>     Startup finished in 1.801s (kernel) + 12.287s (initrd) + 47.925s (userspace) = 1min 2.013s
>     Startup finished in 1.812s (kernel) + 12.302s (initrd) + 47.995s (userspace) = 1min 2.111s
>
>     # -fstack-protector-strong
>     Startup finished in 1.744s (kernel) + 11.223s (initrd) + 44.688s (userspace) = 57.657s
>     Startup finished in 1.721s (kernel) + 11.222s (initrd) + 44.194s (userspace) = 57.138s
>     Startup finished in 1.693s (kernel) + 11.250s (initrd) + 44.426s (userspace) = 57.370s
>
>     # -fstack-protector
>     Startup finished in 1.705s (kernel) + 11.409s (initrd) + 43.563s (userspace) = 56.677s
>     Startup finished in 1.877s (kernel) + 11.137s (initrd) + 43.719s (userspace) = 56.734s
>     Startup finished in 1.708s (kernel) + 11.141s (initrd) + 43.628s (userspace) = 56.478s
>
>     # no stack protector
>     Startup finished in 1.743s (kernel) + 11.190s (initrd) + 43.709s (userspace) = 56.643s
>     Startup finished in 1.763s (kernel) + 11.216s (initrd) + 43.767s (userspace) = 56.747s
>     Startup finished in 1.711s (kernel) + 11.283s (initrd) + 43.878s (userspace) = 56.873s
>
> This patch introduces a configure option to disable the stack protector
> entirely, and conditional stack protector flag selection (in order,
> based on availability): -fstack-protector-strong, -fstack-protector-all,
> no stack protector.
>
> Signed-off-by: Steven Noonan <snoonan@amazon.com>
> Cc: Anthony Liguori <aliguori@amazon.com>
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> [Prefer -fstack-protector-all to -fstack-protector, suggested by
>  Laurent Desnogues. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  configure | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/configure b/configure
> index fb3bd05..eb0e7bb 100755
> --- a/configure
> +++ b/configure
> @@ -198,6 +198,7 @@ audio_win_int=""
>  cc_i386=i386-pc-linux-gnu-gcc
>  libs_qga=""
>  debug_info="yes"
> +stack_protector=""
>
>  # Don't accept a target_list environment variable.
>  unset target_list
> @@ -950,6 +951,10 @@ for opt do
>    ;;
>    --disable-werror) werror="no"
>    ;;
> +  --enable-stack-protector) stack_protector="yes"
> +  ;;
> +  --disable-stack-protector) stack_protector="no"
> +  ;;
>    --disable-curses) curses="no"
>    ;;
>    --enable-curses) curses="yes"
> @@ -1219,6 +1224,7 @@ Advanced options (experts only):
>    --disable-sparse         disable sparse checker (default)
>    --disable-strip          disable stripping binaries
>    --disable-werror         disable compilation abort on warning
> +  --disable-stack-protector disable compiler-provided stack protection
>    --disable-sdl            disable SDL
>    --enable-sdl             enable SDL
>    --with-sdlabi            select preferred SDL ABI 1.2 or 2.0
> @@ -1439,9 +1445,15 @@ for flag in $gcc_flags; do
>      fi
>  done
>
> -if compile_prog "-Werror -fstack-protector-all" "" ; then
> -    QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
> -    LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
> +if test "$stack_protector" != "no" ; then
> +  gcc_flags="-fstack-protector-strong -fstack-protector-all"
> +  for flag in $gcc_flags; do
> +    if compile_prog "-Werror $flag" "" ; then
> +      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
> +      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
> +      break
> +    fi
> +  done
>  fi

My understanding is that -fstack-protector, -fstack-protector-strong,
and -fstack-protector-all are strictly ordered in terms of the number
of functions that are checked, so you have changed the default
behavior to check less functions for compilers that support
-fstack-protector-strong.  Is that what you had in mind?

Also aren't there some versions of gcc that have -fstack-protector
but not the other two options?

Thanks,

Laurent

>  # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
> --
> 1.8.5.3
>

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Paolo Bonzini]

Il 28/03/2014 18:41, Laurent Desnogues ha scritto:
>> > +  gcc_flags="-fstack-protector-strong -fstack-protector-all"
>> > +  for flag in $gcc_flags; do
>> > +    if compile_prog "-Werror $flag" "" ; then
>> > +      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
>> > +      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
>> > +      break
>> > +    fi
>> > +  done
>> >  fi
> My understanding is that -fstack-protector, -fstack-protector-strong,
> and -fstack-protector-all are strictly ordered in terms of the number
> of functions that are checked, so you have changed the default
> behavior to check less functions for compilers that support
> -fstack-protector-strong.  Is that what you had in mind?

Yes.  -fstack-protector-all adds protection in places where it doesn't 
really matter, and that's why it has such a high cost.

Paolo

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Noonan, Steven]

On 3/28/14, 10:51 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:

>Il 28/03/2014 18:41, Laurent Desnogues ha scritto:
>>> > +  gcc_flags="-fstack-protector-strong -fstack-protector-all"
>>> > +  for flag in $gcc_flags; do
>>> > +    if compile_prog "-Werror $flag" "" ; then
>>> > +      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
>>> > +      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
>>> > +      break
>>> > +    fi
>>> > +  done
>>> >  fi
>> My understanding is that -fstack-protector, -fstack-protector-strong,
>> and -fstack-protector-all are strictly ordered in terms of the number
>> of functions that are checked, so you have changed the default
>> behavior to check less functions for compilers that support
>> -fstack-protector-strong.  Is that what you had in mind?
>
>Yes.  -fstack-protector-all adds protection in places where it doesn't
>really matter, and that's why it has such a high cost.

Correct, -fstack-protector-all was too high impact. Sadly
-fstack-protector-strong seems to only exist in RedHat-provided compilers,
which I don't always use -- thus the new default this change provides
doesn't really help, so I'd need to just do 'configure
--disable-stack-protector' to avoid the performance penalty.

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Brad Smith]

On 28/03/14 2:04 PM, Noonan, Steven wrote:
> On 3/28/14, 10:51 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>
>> Il 28/03/2014 18:41, Laurent Desnogues ha scritto:
>>>>> +  gcc_flags="-fstack-protector-strong -fstack-protector-all"
>>>>> +  for flag in $gcc_flags; do
>>>>> +    if compile_prog "-Werror $flag" "" ; then
>>>>> +      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
>>>>> +      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
>>>>> +      break
>>>>> +    fi
>>>>> +  done
>>>>>   fi
>>> My understanding is that -fstack-protector, -fstack-protector-strong,
>>> and -fstack-protector-all are strictly ordered in terms of the number
>>> of functions that are checked, so you have changed the default
>>> behavior to check less functions for compilers that support
>>> -fstack-protector-strong.  Is that what you had in mind?
>>
>> Yes.  -fstack-protector-all adds protection in places where it doesn't
>> really matter, and that's why it has such a high cost.
>
> Correct, -fstack-protector-all was too high impact. Sadly
> -fstack-protector-strong seems to only exist in RedHat-provided compilers,
> which I don't always use -- thus the new default this change provides
> doesn't really help, so I'd need to just do 'configure
> --disable-stack-protector' to avoid the performance penalty.

-fstack-protector-strong exists in OpenBSD's GCC and now LLVM too.

I'd very much be interested in seeing this go in as we're already
using -strong in our own package.

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Peter Maydell]

On 28 March 2014 16:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Steven Noonan <steven@uplinklabs.net>
>
> The -fstack-protector flag family is useful for ensuring safety and for
> debugging, but has a performance impact. Here are some boot time comparisons of
> the various versions of -fstack-protector using qemu-system-arm on an x86_64
> host:

Applied to master, thanks.

-- PMM

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Peter Maydell]

On 28 March 2014 16:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This patch introduces a configure option to disable the stack protector
> entirely, and conditional stack protector flag selection (in order,
> based on availability): -fstack-protector-strong, -fstack-protector-all,
> no stack protector.

I've just noticed that this test doesn't correctly handle MacOSX clang.
For some reason that has this behaviour:

manooth$ clang -o /tmp/zz9 -Werror -fstack-protector /tmp/zz9.c
# OK, plain -fstack-protector works
manooth$ clang -o /tmp/zz9 -Werror -fstack-protector-strong  /tmp/zz9.c
clang: error: argument unused during compilation: '-fstack-protector-strong'
# The strong variant isn't implemented
manooth$ clang -o /tmp/zz9 -Werror -fstack-protector-strong
/tmp/zz9.c -framework CoreFoundation
# ...but for some reason adding the -framework CoreFoundation argument
# suppresses the error!

This is bad because we have that framework argument as part of our
linker flags. Effectively this means that clang won't warn about the
argument at link time but will warn for every .c->.o compile (as well
as ending up with no stack protection).

Changing the test from doing a compile-and-link to just
compiling a single object seems to fix this:

manooth$ git diff
diff --git a/configure b/configure
index eb0e7bb..c85475f 100755
--- a/configure
+++ b/configure
@@ -1448,7 +1448,7 @@ done
 if test "$stack_protector" != "no" ; then
   gcc_flags="-fstack-protector-strong -fstack-protector-all"
   for flag in $gcc_flags; do
-    if compile_prog "-Werror $flag" "" ; then
+    if do_cc $QEMU_CFLAGS -Werror $flag -c -o $TMPO $TMPC ; then
       QEMU_CFLAGS="$QEMU_CFLAGS $flag"
       LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
       break

However perhaps the correct fix is to make MacOSX put
the -framework options in CFLAGS, not LDFLAGS -- they
seem (from what I can gather from google, which is not much)
to be a sort of combination of include files and libraries so should
probably be consistently specified everywhere.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Noonan, Steven]

On Tue, Apr 08, 2014 at 09:37:27PM +0100, Peter Maydell wrote:
> On 28 March 2014 16:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > This patch introduces a configure option to disable the stack protector
> > entirely, and conditional stack protector flag selection (in order,
> > based on availability): -fstack-protector-strong, -fstack-protector-all,
> > no stack protector.
> 
> I've just noticed that this test doesn't correctly handle MacOSX clang.
> For some reason that has this behaviour:
> 
> manooth$ clang -o /tmp/zz9 -Werror -fstack-protector /tmp/zz9.c
> # OK, plain -fstack-protector works
> manooth$ clang -o /tmp/zz9 -Werror -fstack-protector-strong  /tmp/zz9.c
> clang: error: argument unused during compilation: '-fstack-protector-strong'
> # The strong variant isn't implemented
> manooth$ clang -o /tmp/zz9 -Werror -fstack-protector-strong
> /tmp/zz9.c -framework CoreFoundation
> # ...but for some reason adding the -framework CoreFoundation argument
> # suppresses the error!

Oh good grief. This is no doubt Apple's Xcode-provided Clang with all
their Apple patches applied. Stock Clang would break if you added
-framework, I believe.

> This is bad because we have that framework argument as part of our
> linker flags. Effectively this means that clang won't warn about the
> argument at link time but will warn for every .c->.o compile (as well
> as ending up with no stack protection).

So -framework is designed to transparently add the appropriate -I and
-L/-l flags, pointing to the insides of a .framework bundle.

To me, the -framework arguments belong in CFLAGS and LIBS, but not
LDFLAGS. In the context of QEMU's configure script, I think it'd be
QEMU_INCLUDES and LIBS.

> Changing the test from doing a compile-and-link to just
> compiling a single object seems to fix this:
> 
> manooth$ git diff
> diff --git a/configure b/configure
> index eb0e7bb..c85475f 100755
> --- a/configure
> +++ b/configure
> @@ -1448,7 +1448,7 @@ done
>  if test "$stack_protector" != "no" ; then
>    gcc_flags="-fstack-protector-strong -fstack-protector-all"
>    for flag in $gcc_flags; do
> -    if compile_prog "-Werror $flag" "" ; then
> +    if do_cc $QEMU_CFLAGS -Werror $flag -c -o $TMPO $TMPC ; then
>        QEMU_CFLAGS="$QEMU_CFLAGS $flag"
>        LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
>        break
> 
> However perhaps the correct fix is to make MacOSX put
> the -framework options in CFLAGS, not LDFLAGS -- they
> seem (from what I can gather from google, which is not much)
> to be a sort of combination of include files and libraries so should
> probably be consistently specified everywhere.
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Peter Maydell]

On 8 April 2014 21:47, Noonan, Steven <snoonan@amazon.com> wrote:
> On Tue, Apr 08, 2014 at 09:37:27PM +0100, Peter Maydell wrote:
>> This is bad because we have that framework argument as part of our
>> linker flags. Effectively this means that clang won't warn about the
>> argument at link time but will warn for every .c->.o compile (as well
>> as ending up with no stack protection).
>
> So -framework is designed to transparently add the appropriate -I and
> -L/-l flags, pointing to the insides of a .framework bundle.
>
> To me, the -framework arguments belong in CFLAGS and LIBS, but not
> LDFLAGS. In the context of QEMU's configure script, I think it'd be
> QEMU_INCLUDES and LIBS.

Unfortunately, putting "-framework CoreFoundation" in CFLAGS
produces a different warning:

manooth$ clang -o /tmp/zz9.o -Werror -fstack-protector -c /tmp/zz9.c
-framework CoreFoundation
clang: error: -framework CoreFoundation: 'linker' input unused

which would seem to imply that you shouldn't be passing it on
the .c->.o compile command line.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Noonan, Steven]

On Wed, Apr 09, 2014 at 08:40:50AM +0100, Peter Maydell wrote:
> On 8 April 2014 21:47, Noonan, Steven <snoonan@amazon.com> wrote:
> > On Tue, Apr 08, 2014 at 09:37:27PM +0100, Peter Maydell wrote:
> >> This is bad because we have that framework argument as part of our
> >> linker flags. Effectively this means that clang won't warn about the
> >> argument at link time but will warn for every .c->.o compile (as well
> >> as ending up with no stack protection).
> >
> > So -framework is designed to transparently add the appropriate -I and
> > -L/-l flags, pointing to the insides of a .framework bundle.
> >
> > To me, the -framework arguments belong in CFLAGS and LIBS, but not
> > LDFLAGS. In the context of QEMU's configure script, I think it'd be
> > QEMU_INCLUDES and LIBS.
> 
> Unfortunately, putting "-framework CoreFoundation" in CFLAGS
> produces a different warning:
> 
> manooth$ clang -o /tmp/zz9.o -Werror -fstack-protector -c /tmp/zz9.c
> -framework CoreFoundation
> clang: error: -framework CoreFoundation: 'linker' input unused
> 
> which would seem to imply that you shouldn't be passing it on
> the .c->.o compile command line.

I think it's a warning being turned into an error with -Werror. In any
case, it's rightfully complaining, I'd forgotten that '-framework' was
link-only. It's been a while since I built anything serious outside of
command-line apps on Macs.

There are a few relevant command-line arguments that need to be utilized
correctly, though some might not matter in your case:

    -mmacosx-version-min=<version>
	-isysroot <SDK root path>
	-F<framework path>
	-framework <framework name>

The -mmacosx-version-min and -isysroot arguments only are significant if
you intend to redistribute the binary, as they determine what Mac OS X
SDK you're targeting.

For compile command lines, you would need -F arguments to help it
resolve the framework paths (excluding paths for system-provided
frameworks, such as CoreFoundation). Any #include directives for
framework headers would be written as <framework-name/header> e.g.

	#include <CoreFoundation/CoreFoundation.h>

Then for the link command line, you use the same -F arguments to set up
the framework search paths, then add -framework for linkage.

So in your case all you probably need is to drop the -framework
arguments from CFLAGS and plop them into LIBS, and you're probably good
to go.

Re: [Qemu-devel] [PATCH for-2.0] configure: add option to disable -fstack-protector flags

[Peter Maydell]

On 9 April 2014 10:29, Noonan, Steven <snoonan@amazon.com> wrote:
> So in your case all you probably need is to drop the -framework
> arguments from CFLAGS and plop them into LIBS, and you're probably good
> to go.

This is where they are already. The problem is that putting
-framework on the linker command line causes clang to
fail to reject -fsome-random-unknown-thing, which means
we can't do configure detection of -fsomething arguments
using compile_prog  :-(

I think the only thing we can do about this is to make sure
that our configure code to check for -fsomething does a
compile-only check and not a link.

thanks
-- PMM