diff mbox series

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

Message ID 20201214052304.177490-1-siddhesh@sourceware.org
State Superseded
Headers
Series [v2,1/2] string: _FORTIFY_SOURCE=3 using __builtin_dynamic_object_size |

Commit Message

Siddhesh Poyarekar Dec. 14, 2020, 5:23 a.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.
---

Changes from v1:

- Reworded NEWS and creature.texi
- Updated macro soup to warn on _FORTIFY_SOURCE=3 when the compiler is
  not clang.

 NEWS                            |  5 +++++
 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, 41 insertions(+), 18 deletions(-)

Comments

Florian Weimer Dec. 14, 2020, 2:54 p.m. UTC | #1 
* Siddhesh Poyarekar via Libc-alpha:

> diff --git a/NEWS b/NEWS
> index 0820984547..4167f34c13 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -28,6 +28,11 @@ 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 be computationally expensive.  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.

I think it's still a fixed overhead per call (i.e., not proportional to
buffer size or stack depth).  So the performance warning is perhaps a
bit too strong (likewise in the manual).

Rest of the patch looks okay to me.  Debian Code Search doesn't show any
users of __objsize0 reference, so the macro name should be safe,
although it does not have a __glibc_ prefix.

Thanks,
Florian
Jakub Jelinek Dec. 14, 2020, 3:02 p.m. UTC | #2 
On Mon, Dec 14, 2020 at 03:54:11PM +0100, Florian Weimer wrote:
> > diff --git a/NEWS b/NEWS
> > index 0820984547..4167f34c13 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -28,6 +28,11 @@ 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 be computationally expensive.  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.
> 
> I think it's still a fixed overhead per call (i.e., not proportional to
> buffer size or stack depth).  So the performance warning is perhaps a
> bit too strong (likewise in the manual).

It is definitely not a fixed overhead per call, it is an overhead on
everything that performs address arithmetics on the affected pointers, any
PHI nodes for those pointers etc.
Basically, everything that would feed into a particular
__builtin_dynamic_object_size performs exact rather than approximate size
computation and effectively turns all the related code into mudflap-like or
other forms of bounds-checking code.

	Jakub
Siddhesh Poyarekar Dec. 14, 2020, 3:09 p.m. UTC | #3 
On 12/14/20 8:24 PM, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar via Libc-alpha:
> 
>> diff --git a/NEWS b/NEWS
>> index 0820984547..4167f34c13 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -28,6 +28,11 @@ 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 be computationally expensive.  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.
> 
> I think it's still a fixed overhead per call (i.e., not proportional to
> buffer size or stack depth).  So the performance warning is perhaps a
> bit too strong (likewise in the manual).

How about:

s/be computationally expensive/have a performance overhead/ ?

I agree with Jakub that it's not fixed but I also agree that there's 
scope to soften "computationally expensive".

Siddhesh
Florian Weimer Dec. 14, 2020, 3:34 p.m. UTC | #4 
* Siddhesh Poyarekar via Libc-alpha:

> On 12/14/20 8:24 PM, Florian Weimer via Libc-alpha wrote:
>> * Siddhesh Poyarekar via Libc-alpha:
>> 
>>> diff --git a/NEWS b/NEWS
>>> index 0820984547..4167f34c13 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -28,6 +28,11 @@ 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 be computationally expensive.  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.
>> I think it's still a fixed overhead per call (i.e., not proportional
>> to
>> buffer size or stack depth).  So the performance warning is perhaps a
>> bit too strong (likewise in the manual).
>
> How about:
>
> s/be computationally expensive/have a performance overhead/ ?
>
> I agree with Jakub that it's not fixed but I also agree that there's
> scope to soften "computationally expensive".

“have an additional performance overhead”?

Thanks,
Florian
Andreas Schwab Dec. 14, 2020, 3:47 p.m. UTC | #5 
On Dez 14 2020, Florian Weimer via Libc-alpha wrote:

> “have an additional performance overhead”?

Isn't an overhead always in addition?

Andreas.
Florian Weimer Dec. 14, 2020, 3:52 p.m. UTC | #6 
* Andreas Schwab:

> On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
>
>> “have an additional performance overhead”?
>
> Isn't an overhead always in addition?

Compared to level 2?

Thanks,
Florian
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 0820984547..4167f34c13 100644
--- a/NEWS
+++ b/NEWS
@@ -28,6 +28,11 @@  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 be computationally expensive.  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..f2ca61451a 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 be more computationally expensive.
 @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