From patchwork Mon Jul 10 21:18:22 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "H.J. Lu" X-Patchwork-Id: 21532 Received: (qmail 71681 invoked by alias); 10 Jul 2017 21:18:28 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 71668 invoked by uid 89); 10 Jul 2017 21:18:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy= X-HELO: mail-oi0-f42.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=ix9ah9B5VKA4xp+v0ijke56ZgjHRhnHoGKmb345SLlg=; b=CG/WHPjg2GzDIk5EjQEVhsQ5limSHlQY6BFjzWVDA3u1EtmvhG5xUepqCtCbhxZ5/4 Xa1NLdbmjjHT+PCYOTbHZ2vG3J7+fz33rxDvqzvxYR5MlynrpyGQ1xYWyTuLYMBytBPk Be+czvZ3oueFFW2o/zkUdTEI20/A/ZZDaI6R0ATXKIyDctYuXttJ2t+SLjW1lP8P0RFX MgTq+W98aQw91H+fQWiLhr/kuSDndNqicO0GtWif84A9BJ7LnLyUdrgvjKZpgL77dnvl iO8U5S/5f7udPm5jc3d1qZjWLpSj4DgfhU02d5o+8eW8LvNBPP9NdAcCrG09dLZEeNg6 20wg== X-Gm-Message-State: AIVw112X7qXJZ5lFJEy0yrkkO7LtzWL2pq2NmIUIIaNNcfb0RYmVopye A4igLdiDwXYJ3fdIS6tFc4T30BWQlw== X-Received: by 10.202.8.85 with SMTP id 82mr10052154oii.195.1499721503318; Mon, 10 Jul 2017 14:18:23 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20170710200054.GA11938@gmail.com> From: "H.J. Lu" Date: Mon, 10 Jul 2017 14:18:22 -0700 Message-ID: Subject: Re: [PATCH] Avoid backtrace from __stack_chk_fail [BZ #12189] To: Adhemerval Zanella Cc: GNU C Library On Mon, Jul 10, 2017 at 1:45 PM, H.J. Lu wrote: > On Mon, Jul 10, 2017 at 1:39 PM, Adhemerval Zanella > wrote: >> >> >> On 10/07/2017 17:21, H.J. Lu wrote: >>> On Mon, Jul 10, 2017 at 1:14 PM, Joseph Myers wrote: >>>> On Mon, 10 Jul 2017, H.J. Lu wrote: >>>> >>>>> diff --git a/debug/tst-ssp-1.c b/debug/tst-ssp-1.c >>>>> new file mode 100644 >>>>> index 0000000..14d8e26 >>>>> --- /dev/null >>>>> +++ b/debug/tst-ssp-1.c >>>>> @@ -0,0 +1,40 @@ >>>>> +/* Copyright (C) 2017 Free Software Foundation, Inc. >>>> All new files should have a descriptive comment on their first line, >>>> before the copyright notice. >>>> >>> Here is the updated patch. OK for master? >>> >>> >>> -- H.J. >>> >>> >>> 0001-Avoid-backtrace-from-__stack_chk_fail-BZ-12189.patch >>> >>> >>> From 63fc15cbda7fcdeb5e8dcc44c416b6a330cbbf70 Mon Sep 17 00:00:00 2001 >>> From: "H.J. Lu" >>> Date: Mon, 10 Jul 2017 12:21:39 -0700 >>> Subject: [PATCH] Avoid backtrace from __stack_chk_fail [BZ #12189] >>> >>> __stack_chk_fail is called on corrupted stack. __stack_chk_fail should >>> use as little stack as possible. __libc_message is extended to avoid >>> calling BEFORE_ABORT when do_abort >= 3 and __fortify_fail_abort is >>> added to avoid backtrace from __stack_chk_fail. >>> >>> [BZ #12189] >>> * debug/Makefile (CFLAGS-tst-ssp-1.c): New. >>> (tests): Add tst-ssp-1 if -fstack-protector works. >>> * debug/fortify_fail.c (_fortify_fail_abort): New function. >>> (__fortify_fail): Call _fortify_fail_abort. >>> (__fortify_fail_abort): Add a hidden definition. >>> * debug/stack_chk_fail.c (__stack_chk_fail): Call >>> __fortify_fail_abort, instead of __fortify_fail. >>> * debug/tst-ssp-1.c: New file. >>> * include/stdio.h (__fortify_fail_abort): New hidden prototype. >>> * sysdeps/posix/libc_fatal.c (__libc_message): Call BEFORE_ABORT >>> if do_abort < 3. >>> --- >>> debug/Makefile | 7 +++++++ >>> debug/fortify_fail.c | 13 +++++++++++-- >>> debug/stack_chk_fail.c | 2 +- >>> debug/tst-ssp-1.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> include/stdio.h | 3 +++ >>> sysdeps/posix/libc_fatal.c | 3 ++- >>> 6 files changed, 63 insertions(+), 4 deletions(-) >>> create mode 100644 debug/tst-ssp-1.c >>> >>> diff --git a/debug/Makefile b/debug/Makefile >>> index cd4975c..10f9dea 100644 >>> --- a/debug/Makefile >>> +++ b/debug/Makefile >>> @@ -139,12 +139,19 @@ LDFLAGS-tst-backtrace4 = -rdynamic >>> LDFLAGS-tst-backtrace5 = -rdynamic >>> LDFLAGS-tst-backtrace6 = -rdynamic >>> >>> +CFLAGS-tst-ssp-1.c = -Wno-format -Wno-deprecated-declarations -Wno-error \ >>> + -O0 -fstack-protector >> >> Why explicit -O0 here? > > GCC 6.3 may optimize out __stack_chk_fail: > > [hjl@gnu-6 tmp]$ cat ssp-1.c > int main (void) > { > int i = 0; > char foo[30]; > > /* Overflow buffer. */ > for (i = 0; i < 400; i++) > foo[i] = 42; > > return 1; /* fail */ > } > [hjl@gnu-6 tmp]$ gcc -S -O2 -fstack-protector ssp-1.c > ssp-1.c: In function ‘main’: > ssp-1.c:8:12: warning: iteration 30 invokes undefined behavior > [-Waggressive-loop-optimizations] > foo[i] = 42; > ~~~~~~~^~~~ > ssp-1.c:7:3: note: within this loop > for (i = 0; i < 400; i++) > ^~~ > [hjl@gnu-6 tmp]$ cat ssp-1.s > .file "ssp-1.c" > .section .text.startup,"ax",@progbits > .p2align 4,,15 > .globl main > .type main, @function > main: > .LFB0: > .cfi_startproc > movl $1, %eax > ret > .cfi_endproc > .LFE0: > .size main, .-main > .ident "GCC: (GNU) 6.3.1 20170216 (Red Hat 6.3.1-3)" > .section .note.GNU-stack,"",@progbits > [hjl@gnu-6 tmp]$ > >>> + >>> tests = backtrace-tst tst-longjmp_chk tst-chk1 tst-chk2 tst-chk3 \ >>> tst-lfschk1 tst-lfschk2 tst-lfschk3 test-strcpy_chk test-stpcpy_chk \ >>> tst-chk4 tst-chk5 tst-chk6 tst-lfschk4 tst-lfschk5 tst-lfschk6 \ >>> tst-longjmp_chk2 tst-backtrace2 tst-backtrace3 tst-backtrace4 \ >>> tst-backtrace5 tst-backtrace6 >>> >>> +ifeq ($(have-ssp),yes) >>> +tests += tst-ssp-1 >>> +endif >>> + >>> ifeq (,$(CXX)) >>> tests-unsupported = tst-chk4 tst-chk5 tst-chk6 \ >>> tst-lfschk4 tst-lfschk5 tst-lfschk6 >>> diff --git a/debug/fortify_fail.c b/debug/fortify_fail.c >>> index a31977a..13063bf 100644 >>> --- a/debug/fortify_fail.c >>> +++ b/debug/fortify_fail.c >>> @@ -23,11 +23,20 @@ extern char **__libc_argv attribute_hidden; >>> >>> void >>> __attribute__ ((noreturn)) internal_function >>> -__fortify_fail (const char *msg) >>> +__fortify_fail_abort (int no_backtrace, const char *msg) >> >> Would be better to use bool since it is an internal function? > > I thought about it. I wanted to avoid including in > include/stdio.h. I can add it if it is desirable. > >>> { >>> /* The loop is added only to keep gcc happy. */ >>> while (1) >>> - __libc_message (2, "*** %s ***: %s terminated\n", >>> + __libc_message (no_backtrace ? 3 : 2, "*** %s ***: %s terminated\n", >>> msg, __libc_argv[0] ?: ""); >>> } >>> + >>> +void >>> +__attribute__ ((noreturn)) internal_function >>> +__fortify_fail (const char *msg) >>> +{ >>> + __fortify_fail_abort (0, msg); >>> +} >>> + >>> libc_hidden_def (__fortify_fail) >>> +libc_hidden_def (__fortify_fail_abort) >>> diff --git a/debug/stack_chk_fail.c b/debug/stack_chk_fail.c >>> index 120d269..2f39e43 100644 >>> --- a/debug/stack_chk_fail.c >>> +++ b/debug/stack_chk_fail.c >>> @@ -25,7 +25,7 @@ void >>> __attribute__ ((noreturn)) >>> __stack_chk_fail (void) >>> { >>> - __fortify_fail ("stack smashing detected"); >>> + __fortify_fail_abort (1, "stack smashing detected"); >>> } >>> >>> #ifdef SHARED >>> diff --git a/debug/tst-ssp-1.c b/debug/tst-ssp-1.c >>> new file mode 100644 >>> index 0000000..789b035 >>> --- /dev/null >>> +++ b/debug/tst-ssp-1.c >>> @@ -0,0 +1,39 @@ >>> +/* Verify that __stack_chk_fail won't segfault. >>> + Copyright (C) 2017 Free Software Foundation, Inc. >>> + This file is part of the GNU C Library. >>> + >>> + The GNU C Library is free software; you can redistribute it and/or >>> + modify it under the terms of the GNU Lesser General Public >>> + License as published by the Free Software Foundation; either >>> + version 2.1 of the License, or (at your option) any later version. >>> + >>> + The GNU C Library is distributed in the hope that it will be useful, >>> + but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + Lesser General Public License for more details. >>> + >>> + You should have received a copy of the GNU Lesser General Public >>> + License along with the GNU C Library; if not, see >>> + . */ >>> + >>> +/* Based on gcc.dg/ssp-1.c from GCC testsuite. */ >>> + >>> +#include >>> + >>> +static int do_test (void); >>> +#define TEST_FUNCTION do_test () >>> +#define EXPECTED_SIGNAL SIGABRT >>> +#include "../test-skeleton.c" >> >> Use libsupport. > > I will take a look. > Done. I am enclosing 2 patches. The second one uses bool in __fortify_fail_abort. If both are approved, I will squash them into one before commit. Thanks. From 58b69898e9adecefe85c1a421d287a800d974f1b Mon Sep 17 00:00:00 2001 From: "H.J. Lu" Date: Mon, 10 Jul 2017 14:14:27 -0700 Subject: [PATCH 2/2] Replace int with bool in __fortify_fail_abort * debug/fortify_fail.c (__fortify_fail_abort): Replace int with bool. (__fortify_fail): Pass false to __fortify_fail_abort. * debug/stack_chk_fail.c (__stack_chk_fail): Pass true to __fortify_fail_abort. * include/stdio.h: Include l (__fortify_fail_abort): Replace int with bool. --- debug/fortify_fail.c | 4 ++-- debug/stack_chk_fail.c | 2 +- include/stdio.h | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/debug/fortify_fail.c b/debug/fortify_fail.c index 13063bf..cbd0bc2 100644 --- a/debug/fortify_fail.c +++ b/debug/fortify_fail.c @@ -23,7 +23,7 @@ extern char **__libc_argv attribute_hidden; void __attribute__ ((noreturn)) internal_function -__fortify_fail_abort (int no_backtrace, const char *msg) +__fortify_fail_abort (bool no_backtrace, const char *msg) { /* The loop is added only to keep gcc happy. */ while (1) @@ -35,7 +35,7 @@ void __attribute__ ((noreturn)) internal_function __fortify_fail (const char *msg) { - __fortify_fail_abort (0, msg); + __fortify_fail_abort (false, msg); } libc_hidden_def (__fortify_fail) diff --git a/debug/stack_chk_fail.c b/debug/stack_chk_fail.c index 2f39e43..5ecb7bc 100644 --- a/debug/stack_chk_fail.c +++ b/debug/stack_chk_fail.c @@ -25,7 +25,7 @@ void __attribute__ ((noreturn)) __stack_chk_fail (void) { - __fortify_fail_abort (1, "stack smashing detected"); + __fortify_fail_abort (true, "stack smashing detected"); } #ifdef SHARED diff --git a/include/stdio.h b/include/stdio.h index 31dc7a6..7467b82 100644 --- a/include/stdio.h +++ b/include/stdio.h @@ -1,5 +1,6 @@ #ifndef _STDIO_H # include +# include # ifndef _ISOMAC /* Now define the internal interfaces. */ @@ -92,7 +93,7 @@ extern void __libc_fatal (const char *__message) extern void __libc_message (int do_abort, const char *__fnt, ...); extern void __fortify_fail (const char *msg) __attribute__ ((__noreturn__)) internal_function; -extern void __fortify_fail_abort (int, const char *msg) +extern void __fortify_fail_abort (bool, const char *msg) __attribute__ ((__noreturn__)) internal_function; libc_hidden_proto (__fortify_fail) libc_hidden_proto (__fortify_fail_abort) -- 2.9.4