From patchwork Mon Nov 30 08:40:53 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siddhesh Poyarekar X-Patchwork-Id: 41233 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 62B3F383F852; Mon, 30 Nov 2020 08:41:34 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 62B3F383F852 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1606725694; bh=TM0ct7NkuMe3E/Fk8RJuoWQahSEjjDIxe/3xhQHMFGo=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=fp13N+acoWDKKlhZF4C/UqL5Dbc7PLZy+C6JPeExWZ6aZ3tRas1RekZV3EMll1UQL 1a3d6pxk74E9D+ukg8KvRROqkeOF81r1VYma/Er7SD2/HloSGjq+RfGKQiNiepSmZ6 wK8tZ0WowqKrMue36LrnQ9YrS+M+wHDHX4zEmvI4= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from aye.elm.relay.mailchannels.net (aye.elm.relay.mailchannels.net [23.83.212.6]) by sourceware.org (Postfix) with ESMTPS id 8535C3857810 for ; Mon, 30 Nov 2020 08:41:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8535C3857810 X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 5161032179E; Mon, 30 Nov 2020 08:41:23 +0000 (UTC) Received: from pdx1-sub0-mail-a90.g.dreamhost.com (100-100-138-54.trex.outbound.svc.cluster.local [100.100.138.54]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 6B5083205A7; Mon, 30 Nov 2020 08:41:22 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a90.g.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384) by 0.0.0.0:2500 (trex/5.18.10); Mon, 30 Nov 2020 08:41:23 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Shrill-Print: 062a5dfe7186a8b8_1606725682950_2361871813 X-MC-Loop-Signature: 1606725682950:1840534485 X-MC-Ingress-Time: 1606725682950 Received: from pdx1-sub0-mail-a90.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a90.g.dreamhost.com (Postfix) with ESMTP id 22DCA85986; Mon, 30 Nov 2020 00:41:22 -0800 (PST) Received: from rhbox.redhat.com (unknown [1.186.101.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a90.g.dreamhost.com (Postfix) with ESMTPSA id C96AE7F25D; Mon, 30 Nov 2020 00:41:18 -0800 (PST) X-DH-BACKEND: pdx1-sub0-mail-a90 To: libc-alpha@sourceware.org Subject: [PATCH RFC] __builtin_dynamic_object_size with -D_FORTIFY_SOURCE=3 Date: Mon, 30 Nov 2020 14:10:53 +0530 Message-Id: <20201130084053.90470-1-siddhesh@sourceware.org> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_NUMSUBJECT, KAM_SHORT, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NEUTRAL, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Siddhesh Poyarekar via Libc-alpha From: Siddhesh Poyarekar Reply-To: Siddhesh Poyarekar Cc: fweimer@redhat.com, jakub@redhat.com Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" I tried to incorporate the __builtin_dynamic_object_size[1] builtin available in clang to enhance _FORTIFY_SOURCE and was looking for some early feedback so that I can incorporate it into the final patch that I hope to come up with soon. __builtin_dynamic_object_size ----------------------------- __builtin_dynamic_object_size is intended to be a drop-in replacement for __builtin_object_size (more on why it isn't at the moment, but later) that goes a little further. 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 by a bit. In practice, it can find these expressions through malloc/calloc calls earlier in the translation unit. 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: extern void *getmem (size_t); void *copy_obj (const void *src, size_t alloc, size_t copysize) { void *obj = malloc (alloc); memcpy (obj, src, copysize); return obj; } Note however that 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. -D_FORTIFY_SOURCE=3 ------------------- I restricted usage of __builtin_dynamic_object_size to a new _FORTIFY_SOURCE level because it has one fundamental difference from the earlier level, which is that it can pass a variable to the _chk function instead of constant. As a result, 1 and 2 are fast with most overhead being optimized out while 3 may have additional overhead. For example at the moment the simple case of memcpy, memmove, etc. where they're implemented with compiler builtins like __builtin___memcpy_chk, clang is able to generate compact code that passes the expression to __memcpy_chk or just generates a memcpy when possible. In the non-builtin cases though, where the size evaluates to an expression, one may see patterns like: cmpq $-1, %rbx je .LBB0_2 callq fortified_chk jmp .LBB0_3 .LBB0_2: # %if.else callq fortified since the compiler isn't smart enough yet to reduce that condition. This can be fixed (I'm working on that right now) in the simple case of a direct comparison and thus work for _FORTIFY_SOURCE=3, but it may be harder to evaluate for __builtin_dynamic_object_size in general where the comparison happens indirectly. This is also why __builtin_dynamic_object_size isn't exactly a drop-in replacement for __builtin_object_size in all cases. Outstanding Issues/RFC: ------------------ - As mentioned above, I'm trying to get clang to eliminate that conditional so that it can be applied more widely beyond to __builtin___func_chk like functions. That hopefully won't be too hard. However there is a risk that the llvm project rejects such a peephole optimisation; in that case would the additional overhead of the conditional be acceptable? It is a separate fortify level, so it may be easier to justify. - gcc does not support __builtin_dynamic_object_size yet. Based on previous discussions[2] ISTM that the idea hasn't bee rejected outright, just that the gcc community would like to see a more detailed specification before committing to add it. I think the glibc implementation will help us arrive at that, although there are issues beyond glibc usage of __builtin_dynamic_object_size or __builtin_object_size. I hope to start discussions on this next year, nearer to the next gcc stage 1 open. - The idea of -D_FORTIFY_SOURCE=3 is a beginning of a shift in design. This introduces hardening that could potentially have noticeable additional overhead at runtime. It does expand coverage of fortification and there's potential for additional coverage on similar lines. Is it enough to simply increment the fortify level or should this be a different feature macro altogether? Maybe incrementing the fortify level is sufficient but we need some additional space to grow static checks, i.e. start dynamic checks from -D_FORTIFY_SOURCE=9? Or any other concerns regarding where such checks ought to fit in? [1] https://reviews.llvm.org/D56760 [2] https://gcc.gnu.org/legacy-ml/gcc/2019-01/msg00188.html --- include/features.h | 6 +++--- misc/sys/cdefs.h | 9 +++++++++ string/bits/string_fortified.h | 24 ++++++++++++------------ string/bits/strings_fortified.h | 4 ++-- 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/include/features.h b/include/features.h index f3e62d3362..86409dd457 100644 --- a/include/features.h +++ b/include/features.h @@ -397,10 +397,10 @@ # 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 -# define __USE_FORTIFY_LEVEL 2 +# elif _FORTIFY_SOURCE > 2 +# define __USE_FORTIFY_LEVEL 3 # else -# define __USE_FORTIFY_LEVEL 1 +# define __USE_FORTIFY_LEVEL _FORTIFY_SOURCE # endif #endif #ifndef __USE_FORTIFY_LEVEL diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index e94d09d7dd..563b238ed5 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 if 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..e8e14506f9 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, @@ -107,7 +107,7 @@ extern char *__REDIRECT_NTH (__stpncpy_alias, (char *__dest, const char *__src, __fortify_function char * __NTH (stpncpy (char *__dest, const char *__src, size_t __n)) { - if (__bos (__dest) != (size_t) -1 + if (__objsize (__dest) != (size_t) -1 && (!__builtin_constant_p (__n) || __n > __bos (__dest))) return __stpncpy_chk (__dest, __src, __n, __bos (__dest)); return __stpncpy_alias (__dest, __src, __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