svn commit: r354669 - head/lib/libc/secure

[Kyle Evans]

Author: kevans
Date: Wed Nov 13 02:14:17 2019
New Revision: 354669
URL: https://svnweb.freebsd.org/changeset/base/354669

Log:
  ssp: add a priority to the __stack_chk_guard constructor
  
  First, this commit is a NOP on GCC <= 4.x; this decidedly doesn't work
  cleanly on GCC 4.2, and it will be gone soon anyways so I chose not to dump
  time into figuring out if there's a way to make it work. xtoolchain-gcc,
  clocking in as GCC6, can cope with it just fine and later versions are also
  generally ok with the syntax. I suspect very few users are running GCC4.2
  built worlds and also experiencing potential fallout from the status quo.
  
  For dynamically linked applications, this change also means very little.
  rtld will run libc ctors before most others, so the situation is
  approximately a NOP for these as well.
  
  The real cause for this change is statically linked applications doing
  almost questionable things in their constructors. qemu-user-static, for
  instance, creates a thread in a global constructor for their async rcu
  callbacks. In general, this works in other places-
  
  - On OpenBSD, __stack_chk_guard is stored in an .openbsd.randomdata section
    that's initialized by the kernel in the static case, or ld.so in the
    dynamic case
  - On Linux, __stack_chk_guard is apparently stored in TLS and such a problem
    is circumvented there because the value is presumed stable in the new
    thread.
  
  On FreeBSD, the rcu thread creation ctor and __guard_setup are both unmarked
  priority. qemu-user-static spins up the rcu thread prior to __guard_setup
  which starts making function calls- some of these are sprinkled with the
  canary. In the middle of one of these functions, __guard_setup is invoked in
  the main thread and __stack_chk_guard changes- qemu-user-static is promptly
  terminated for an SSP violation that didn't actually happen.
  
  This is not an all-too-common problem. We circumvent it here by giving the
  __stack_chk_guard constructor a solid priority. 200 was chosen because that
  gives static applications ample range (down to 101) for working around it
  if they really need to. I suspect most applications will "just work" as
  expected- the default/non-prioritized flavor of __constructor__ functions
  run last, and the canary is generally not expected to change as of this
  point at the very least.
  
  This took approximately three weeks of spare time debugging to pin down.
  
  PR:		241905

Modified:
  head/lib/libc/secure/stack_protector.c

Modified: head/lib/libc/secure/stack_protector.c
==============================================================================
--- head/lib/libc/secure/stack_protector.c	Wed Nov 13 01:58:43 2019	(r354668)
+++ head/lib/libc/secure/stack_protector.c	Wed Nov 13 02:14:17 2019	(r354669)
@@ -40,11 +40,27 @@ __FBSDID("$FreeBSD$");
 #include <unistd.h>
 #include "libc_private.h"
 
+/*
+ * We give __guard_setup a defined priority early on so that statically linked
+ * applications have a defined priority at which __stack_chk_guard will be
+ * getting initialized.  This will not matter to most applications, because
+ * they're either not usually statically linked or they simply don't do things
+ * in constructors that would be adversely affected by their positioning with
+ * respect to this initialization.
+ */
+#if defined(__GNUC__) && __GNUC__ <= 4
+#define	_GUARD_SETUP_CTOR_ATTR	\
+    __attribute__((__constructor__, __used__));
+#else
+#define	_GUARD_SETUP_CTOR_ATTR	 \
+    __attribute__((__constructor__ (200), __used__));
+#endif
+
 extern int __sysctl(const int *name, u_int namelen, void *oldp,
     size_t *oldlenp, void *newp, size_t newlen);
 
 long __stack_chk_guard[8] = {0, 0, 0, 0, 0, 0, 0, 0};
-static void __guard_setup(void) __attribute__((__constructor__, __used__));
+static void __guard_setup(void) _GUARD_SETUP_CTOR_ATTR;
 static void __fail(const char *);
 void __stack_chk_fail(void);
 void __chk_fail(void);