diff mbox series

[v4,1/2] string: _FORTIFY_SOURCE=3 using __builtin_dynamic_object_size

Message ID 20201215182238.2780547-2-siddhesh@sourceware.org
State Superseded
Headers
Series _FORTIFY_SOURCE=3 |

Commit Message

Siddhesh Poyarekar Dec. 15, 2020, 6:22 p.m. UTC 
  Introduce a new _FORTIFY_SOURCE level of 3 to enable additional
fortifications that may have a potential performance impact.  At the
moment this level of fortification involves the use of the
__builtin_dynamic_object_size builtin whenever the compiler supports
it.

This change enhances fortified string functions to use
__builtin_dynamic_object_size under _FORTIFY_SOURCE=3 whenever the
compiler supports it.

__builtin_dynamic_object_size
-----------------------------

__builtin_dynamic_object_size is an LLVM builtin that is similar to
__builtin_object_size.  In addition to what __builtin_object_size
does, i.e. replace the builtin call with a constant object size,
__builtin_dynamic_object_size will replace the call site with an
expression that evaluates to the object size, thus expanding its
applicability.  In practice, __builtin_dynamic_object_size evaluates
these expressions through malloc/calloc calls that it can associate
with the object being evaluated.

A simple motivating example is below; -D_FORTIFY_SOURCE=2 would miss
this and emit memcpy, but -D_FORTIFY_SOURCE=3 with the help of
__builtin_dynamic_object_size is able to emit __memcpy_chk with the
allocation size expression passed into the function:

void *copy_obj (const void *src, size_t alloc, size_t copysize)
{
  void *obj = malloc (alloc);
  memcpy (obj, src, copysize);

  return obj;
}

Limitations
-----------

If the object was allocated elsewhere that the compiler cannot see, or
if it was allocated in the function with a function that the compiler
does not recognize as an allocator then __builtin_dynamic_object_size
also returns -1.

Further, the expression used to compute object size may be non-trivial
and may potentially incur a noticeable performance impact.  These
fortifications are hence enabled at a new _FORTIFY_SOURCE level to
allow developers to make a choice on the tradeoff according to their
environment.
---
 NEWS                            |  6 ++++++
 include/features.h              | 11 +++++++++--
 include/string.h                |  5 +++--
 manual/creature.texi            |  3 ++-
 misc/sys/cdefs.h                |  9 +++++++++
 string/bits/string_fortified.h  | 22 +++++++++++-----------
 string/bits/strings_fortified.h |  4 ++--
 7 files changed, 42 insertions(+), 18 deletions(-)

Comments

Jakub Jelinek Dec. 15, 2020, 6:33 p.m. UTC | #1 
On Tue, Dec 15, 2020 at 11:52:37PM +0530, Siddhesh Poyarekar wrote:
> -# elif _FORTIFY_SOURCE > 1
> +# elif _FORTIFY_SOURCE == 3 && !__glibc_clang_prereq(9, 0)
> +#  warning _FORTIFY_SOURCE > 2 requires LLVM 9.0 or later, falling back to 2

The == 3 above should be really > 2 instead.

>  #  define __USE_FORTIFY_LEVEL 2
> +# elif _FORTIFY_SOURCE > 2
> +#  if __glibc_clang_prereq(9, 0)
> +#   define __USE_FORTIFY_LEVEL 3
> +#  else
> +#   define __USE_FORTIFY_LEVEL 2
> +#  endif

and then I don't really understand the above #if, you know that
__glibc_clang_prereq(9, 0) is already true and so can just define
__USE_FORTIFY_LEVEl to 3.
Though, perhaps better would be to remove the #elif == 3 above and
add the warning next to the define to 2.

>  # else
> -#  define __USE_FORTIFY_LEVEL 1
> +#  define __USE_FORTIFY_LEVEL _FORTIFY_SOURCE

Is it a good idea to define __USE_FORTIFY_LEVEL to _FORTIFY_SOURCE
directly?  I mean, one can e.g. use -D_FORTIFY_SOURCE=2LL etc.
and not sure if all the code that uses it will deal with 2LL properly.

	Jakub
Siddhesh Poyarekar Dec. 15, 2020, 7:01 p.m. UTC | #2 
On 12/16/20 12:03 AM, Jakub Jelinek via Libc-alpha wrote:
> On Tue, Dec 15, 2020 at 11:52:37PM +0530, Siddhesh Poyarekar wrote:
>> -# elif _FORTIFY_SOURCE > 1
>> +# elif _FORTIFY_SOURCE == 3 && !__glibc_clang_prereq(9, 0)
>> +#  warning _FORTIFY_SOURCE > 2 requires LLVM 9.0 or later, falling back to 2
> 
> The == 3 above should be really > 2 instead.

I wrote it this way to maintain consistency with what the code has been 
doing so far, which is to silently lower the spurious high values (e.g. 
_FORTIFY_SOURCE=8) to the nearest supported one and only warn for ==3 
for non-clang.  Do you think we should change that?  If yes then we need 
a couple of levels of warnings, i.e.:

- Warn for too high values in general (i.e. > 3)
- Warn for too high value for specific compilers (i.e. > 2 for gcc)

Does that sound OK?

>>   # else
>> -#  define __USE_FORTIFY_LEVEL 1
>> +#  define __USE_FORTIFY_LEVEL _FORTIFY_SOURCE
> 
> Is it a good idea to define __USE_FORTIFY_LEVEL to _FORTIFY_SOURCE
> directly?  I mean, one can e.g. use -D_FORTIFY_SOURCE=2LL etc.
> and not sure if all the code that uses it will deal with 2LL properly.

OK, I'll fix it up.

Siddhesh
Jakub Jelinek Dec. 15, 2020, 7:05 p.m. UTC | #3 
On Wed, Dec 16, 2020 at 12:31:02AM +0530, Siddhesh Poyarekar wrote:
> On 12/16/20 12:03 AM, Jakub Jelinek via Libc-alpha wrote:
> > On Tue, Dec 15, 2020 at 11:52:37PM +0530, Siddhesh Poyarekar wrote:
> > > -# elif _FORTIFY_SOURCE > 1
> > > +# elif _FORTIFY_SOURCE == 3 && !__glibc_clang_prereq(9, 0)
> > > +#  warning _FORTIFY_SOURCE > 2 requires LLVM 9.0 or later, falling back to 2
> > 
> > The == 3 above should be really > 2 instead.
> 
> I wrote it this way to maintain consistency with what the code has been
> doing so far, which is to silently lower the spurious high values (e.g.
> _FORTIFY_SOURCE=8) to the nearest supported one and only warn for ==3 for
> non-clang.  Do you think we should change that?  If yes then we need a

What you wrote will be silent for -D_FORTIFY_SOURCE=4, but will be noisy
for -D_FORTIFY_SOURCE=3 (both with gcc or with clang < 9).
That is strange.

	Jakub
Siddhesh Poyarekar Dec. 16, 2020, 12:38 a.m. UTC | #4 
On 12/16/20 12:35 AM, Jakub Jelinek via Libc-alpha wrote:
> What you wrote will be silent for -D_FORTIFY_SOURCE=4, but will be noisy
> for -D_FORTIFY_SOURCE=3 (both with gcc or with clang < 9).
> That is strange.

Yeah, that's because 4 and over don't mean anything at the moment, while 
3 does, similar to how it's noisy for gcc < 4.1 for all fortify levels. 
  However, I can make it noisy all the way if that makes sense.

Alternatively, we could just not have those warnings and choose 
fortification levels silently.  I don't have too strong an opinion 
either way, what do you think?

Siddhesh
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 0820984547..df46560878 100644
--- a/NEWS
+++ b/NEWS
@@ -28,6 +28,12 @@  Major new features:
   The 32-bit RISC-V port requires at least Linux 5.4, GCC 7.1 and binutils
   2.28.
 
+* A new fortification level _FORTIFY_SOURCE=3 is available.  At this level,
+  glibc may use additional checks that may have an additional performance
+  overhead.  At present these checks are available only on LLVM 9 and later.
+  The latest GCC available at this time (10.2) does not support this level of
+  fortification.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * The mallinfo function is marked deprecated.  Callers should call
diff --git a/include/features.h b/include/features.h
index f3e62d3362..b1225d5b3f 100644
--- a/include/features.h
+++ b/include/features.h
@@ -397,10 +397,17 @@ 
 #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
 # elif !__GNUC_PREREQ (4, 1)
 #  warning _FORTIFY_SOURCE requires GCC 4.1 or later
-# elif _FORTIFY_SOURCE > 1
+# elif _FORTIFY_SOURCE == 3 && !__glibc_clang_prereq(9, 0)
+#  warning _FORTIFY_SOURCE > 2 requires LLVM 9.0 or later, falling back to 2
 #  define __USE_FORTIFY_LEVEL 2
+# elif _FORTIFY_SOURCE > 2
+#  if __glibc_clang_prereq(9, 0)
+#   define __USE_FORTIFY_LEVEL 3
+#  else
+#   define __USE_FORTIFY_LEVEL 2
+#  endif
 # else
-#  define __USE_FORTIFY_LEVEL 1
+#  define __USE_FORTIFY_LEVEL _FORTIFY_SOURCE
 # endif
 #endif
 #ifndef __USE_FORTIFY_LEVEL
diff --git a/include/string.h b/include/string.h
index 7d344d77d4..841ee05a1d 100644
--- a/include/string.h
+++ b/include/string.h
@@ -123,10 +123,11 @@  libc_hidden_proto (__strerror_l)
 void __explicit_bzero_chk_internal (void *, size_t, size_t)
   __THROW __nonnull ((1)) attribute_hidden;
 # define explicit_bzero(buf, len) \
-  __explicit_bzero_chk_internal (buf, len, __bos0 (buf))
+  __explicit_bzero_chk_internal (buf, len, __objsize0 (buf))
 #elif !IS_IN (nonlib)
 void __explicit_bzero_chk (void *, size_t, size_t) __THROW __nonnull ((1));
-# define explicit_bzero(buf, len) __explicit_bzero_chk (buf, len, __bos0 (buf))
+# define explicit_bzero(buf, len) __explicit_bzero_chk (buf, len,	      \
+							__objsize0 (buf))
 #endif
 
 libc_hidden_builtin_proto (memchr)
diff --git a/manual/creature.texi b/manual/creature.texi
index be5050468b..31208ccb2b 100644
--- a/manual/creature.texi
+++ b/manual/creature.texi
@@ -254,7 +254,8 @@  included.
 @standards{GNU, (none)}
 If this macro is defined to @math{1}, security hardening is added to
 various library functions.  If defined to @math{2}, even stricter
-checks are applied.
+checks are applied. If defined to @math{3}, @theglibc{} may also use
+checks that may have an additional performance overhead.
 @end defvr
 
 @defvr Macro _REENTRANT
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index a06f1cfd91..ca51a5c3ad 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -127,6 +127,15 @@ 
 #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)
 #define __bos0(ptr) __builtin_object_size (ptr, 0)
 
+/* Use __builtin_dynamic_object_size at _FORTIFY_SOURCE=3 when available.  */
+#if __USE_FORTIFY_LEVEL == 3 && __glibc_clang_prereq (9, 0)
+# define __objsize0(__o) __builtin_dynamic_object_size (__o, 0)
+# define __objsize(__o) __builtin_dynamic_object_size (__o, 1)
+#else
+# define __objsize0(__o) __bos0 (__o)
+# define __objsize(__o) __bos (__o)
+#endif
+
 #if __GNUC_PREREQ (4,3)
 # define __warnattr(msg) __attribute__((__warning__ (msg)))
 # define __errordecl(name, msg) \
diff --git a/string/bits/string_fortified.h b/string/bits/string_fortified.h
index 4c1aeb45f1..c9f9197aef 100644
--- a/string/bits/string_fortified.h
+++ b/string/bits/string_fortified.h
@@ -26,13 +26,13 @@  __fortify_function void *
 __NTH (memcpy (void *__restrict __dest, const void *__restrict __src,
 	       size_t __len))
 {
-  return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
+  return __builtin___memcpy_chk (__dest, __src, __len, __objsize0 (__dest));
 }
 
 __fortify_function void *
 __NTH (memmove (void *__dest, const void *__src, size_t __len))
 {
-  return __builtin___memmove_chk (__dest, __src, __len, __bos0 (__dest));
+  return __builtin___memmove_chk (__dest, __src, __len, __objsize0 (__dest));
 }
 
 #ifdef __USE_GNU
@@ -40,7 +40,7 @@  __fortify_function void *
 __NTH (mempcpy (void *__restrict __dest, const void *__restrict __src,
 		size_t __len))
 {
-  return __builtin___mempcpy_chk (__dest, __src, __len, __bos0 (__dest));
+  return __builtin___mempcpy_chk (__dest, __src, __len, __objsize0 (__dest));
 }
 #endif
 
@@ -53,7 +53,7 @@  __NTH (mempcpy (void *__restrict __dest, const void *__restrict __src,
 __fortify_function void *
 __NTH (memset (void *__dest, int __ch, size_t __len))
 {
-  return __builtin___memset_chk (__dest, __ch, __len, __bos0 (__dest));
+  return __builtin___memset_chk (__dest, __ch, __len, __objsize0 (__dest));
 }
 
 #ifdef __USE_MISC
@@ -65,21 +65,21 @@  void __explicit_bzero_chk (void *__dest, size_t __len, size_t __destlen)
 __fortify_function void
 __NTH (explicit_bzero (void *__dest, size_t __len))
 {
-  __explicit_bzero_chk (__dest, __len, __bos0 (__dest));
+  __explicit_bzero_chk (__dest, __len, __objsize0 (__dest));
 }
 #endif
 
 __fortify_function char *
 __NTH (strcpy (char *__restrict __dest, const char *__restrict __src))
 {
-  return __builtin___strcpy_chk (__dest, __src, __bos (__dest));
+  return __builtin___strcpy_chk (__dest, __src, __objsize (__dest));
 }
 
 #ifdef __USE_GNU
 __fortify_function char *
 __NTH (stpcpy (char *__restrict __dest, const char *__restrict __src))
 {
-  return __builtin___stpcpy_chk (__dest, __src, __bos (__dest));
+  return __builtin___stpcpy_chk (__dest, __src, __objsize (__dest));
 }
 #endif
 
@@ -88,14 +88,14 @@  __fortify_function char *
 __NTH (strncpy (char *__restrict __dest, const char *__restrict __src,
 		size_t __len))
 {
-  return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
+  return __builtin___strncpy_chk (__dest, __src, __len, __objsize (__dest));
 }
 
 #if __GNUC_PREREQ (4, 7) || __glibc_clang_prereq (2, 6)
 __fortify_function char *
 __NTH (stpncpy (char *__dest, const char *__src, size_t __n))
 {
-  return __builtin___stpncpy_chk (__dest, __src, __n, __bos (__dest));
+  return __builtin___stpncpy_chk (__dest, __src, __n, __objsize (__dest));
 }
 #else
 extern char *__stpncpy_chk (char *__dest, const char *__src, size_t __n,
@@ -118,7 +118,7 @@  __NTH (stpncpy (char *__dest, const char *__src, size_t __n))
 __fortify_function char *
 __NTH (strcat (char *__restrict __dest, const char *__restrict __src))
 {
-  return __builtin___strcat_chk (__dest, __src, __bos (__dest));
+  return __builtin___strcat_chk (__dest, __src, __objsize (__dest));
 }
 
 
@@ -126,7 +126,7 @@  __fortify_function char *
 __NTH (strncat (char *__restrict __dest, const char *__restrict __src,
 		size_t __len))
 {
-  return __builtin___strncat_chk (__dest, __src, __len, __bos (__dest));
+  return __builtin___strncat_chk (__dest, __src, __len, __objsize (__dest));
 }
 
 #endif /* bits/string_fortified.h */
diff --git a/string/bits/strings_fortified.h b/string/bits/strings_fortified.h
index d4091f4f69..122199e036 100644
--- a/string/bits/strings_fortified.h
+++ b/string/bits/strings_fortified.h
@@ -22,13 +22,13 @@ 
 __fortify_function void
 __NTH (bcopy (const void *__src, void *__dest, size_t __len))
 {
-  (void) __builtin___memmove_chk (__dest, __src, __len, __bos0 (__dest));
+  (void) __builtin___memmove_chk (__dest, __src, __len, __objsize0 (__dest));
 }
 
 __fortify_function void
 __NTH (bzero (void *__dest, size_t __len))
 {
-  (void) __builtin___memset_chk (__dest, '\0', __len, __bos0 (__dest));
+  (void) __builtin___memset_chk (__dest, '\0', __len, __objsize0 (__dest));
 }
 
 #endif