From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38271 invoked by alias); 24 Jul 2017 12:54:48 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 38249 invoked by uid 89); 24 Jul 2017 12:54:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.2 spammy= X-HELO: mail-qt0-f170.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=/svMYECsscSmoZGTbD1aRkZPOW8LzngQs7Y/ecDgje0=; b=hG98+VYK8RZ3Fx3AZn2JRSi8++alqcCTLkS1AL2wFmlzw/CZ8cz7afSwPxpZvcjejh XsIgkx4Y+fLUvCfdh3O3xWHS8Nhv6eI712nC67xblBCfNLpvXv7vUGcvNmOZyTD4e3Oh Eu75azBo7HoWAcG2shnF+q0kqiIgN5AL/7RFlF5UU6t4NOspLXyy593f5IgQtiPrQXG7 +eiIpmuUD/wAbEESECsQGUtLP4Xzb7ZsGXuOq8NWlZhhIcyOl0nHv5fqi4f1YkFb/9oE 6POEDM9T9T/xfSF0JkovU+t8E0/p0qUEEiQgyZwHlycpFzfA/GIhrI0mHy038kxCUxXd Btyg== X-Gm-Message-State: AIVw112IImnvr6ikddabbDjyahRlcBhjQ+E4rlymw6UY61+uoj6aX6Fq Ge4Vm5RqityFE0zEdaXeSw== X-Received: by 10.237.63.226 with SMTP id w31mr8458920qth.6.1500900883666; Mon, 24 Jul 2017 05:54:43 -0700 (PDT) Subject: Re: [PATCH] Avoid accessing corrupted stack from __stack_chk_fail [BZ #21752] To: "H.J. Lu" , GNU C Library References: <20170719185036.GA32763@gmail.com> From: Carlos O'Donell Message-ID: <6a294370-5a48-a38a-f701-513cfa3df7ab@redhat.com> Date: Mon, 24 Jul 2017 13:08:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-07/txt/msg00806.txt.bz2 On 07/24/2017 08:50 AM, H.J. Lu wrote: > On Wed, Jul 19, 2017 at 11:50 AM, H.J. Lu wrote: >> __libc_argv[0] points to address on stack and __libc_secure_getenv >> accesses environment variables which are on stack. We should avoid >> accessing stack when stack is corrupted. >> >> This patch also renames function argument in __fortify_fail_abort >> from do_backtrace to need_backtrace to avoid confusion with do_backtrace >> from enum __libc_message_action. >> >> Tested on x86-64 and i686. OK for master? >> >> H.J. >> --- >> [BZ #21752] >> * debug/fortify_fail.c (__fortify_fail_abort): Don't pass down >> __libc_argv[0] if we aren't doing backtrace. Rename do_backtrace >> to need_backtrace. >> * sysdeps/posix/libc_fatal.c (__libc_message): Don't call >> __libc_secure_getenv if we aren't doing backtrace. >> --- >> debug/fortify_fail.c | 12 ++++++++---- >> sysdeps/posix/libc_fatal.c | 15 ++++++++++----- >> 2 files changed, 18 insertions(+), 9 deletions(-) >> >> diff --git a/debug/fortify_fail.c b/debug/fortify_fail.c >> index c90d384daf..a0777ae570 100644 >> --- a/debug/fortify_fail.c >> +++ b/debug/fortify_fail.c >> @@ -24,13 +24,17 @@ extern char **__libc_argv attribute_hidden; >> >> void >> __attribute__ ((noreturn)) internal_function >> -__fortify_fail_abort (_Bool do_backtrace, const char *msg) >> +__fortify_fail_abort (_Bool need_backtrace, const char *msg) >> { >> - /* The loop is added only to keep gcc happy. */ >> + /* The loop is added only to keep gcc happy. Don't pass down >> + __libc_argv[0] if we aren't doing backtrace since __libc_argv[0] >> + may point to the corrupted stack. */ >> while (1) >> - __libc_message (do_backtrace ? (do_abort | do_backtrace) : do_abort, >> + __libc_message (need_backtrace ? (do_abort | do_backtrace) : do_abort, >> "*** %s ***: %s terminated\n", >> - msg, __libc_argv[0] ?: ""); >> + msg, >> + (need_backtrace && __libc_argv[0] != NULL >> + ? __libc_argv[0] : "")); >> } >> >> void >> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c >> index 25af8bd413..c9189194dd 100644 >> --- a/sysdeps/posix/libc_fatal.c >> +++ b/sysdeps/posix/libc_fatal.c >> @@ -75,11 +75,16 @@ __libc_message (enum __libc_message_action action, const char *fmt, ...) >> FATAL_PREPARE; >> #endif >> >> - /* Open a descriptor for /dev/tty unless the user explicitly >> - requests errors on standard error. */ >> - const char *on_2 = __libc_secure_getenv ("LIBC_FATAL_STDERR_"); >> - if (on_2 == NULL || *on_2 == '\0') >> - fd = open_not_cancel_2 (_PATH_TTY, O_RDWR | O_NOCTTY | O_NDELAY); >> + /* Don't call __libc_secure_getenv if we aren't doing backtrace, which >> + may access the corrupted stack. */ >> + if ((action & do_backtrace)) >> + { >> + /* Open a descriptor for /dev/tty unless the user explicitly >> + requests errors on standard error. */ >> + const char *on_2 = __libc_secure_getenv ("LIBC_FATAL_STDERR_"); >> + if (on_2 == NULL || *on_2 == '\0') >> + fd = open_not_cancel_2 (_PATH_TTY, O_RDWR | O_NOCTTY | O_NDELAY); >> + } >> >> if (fd == -1) >> fd = STDERR_FILENO; >> -- >> 2.13.3 >> > > Any comments, objections? > > Glibc should avoid accessing corrupted stack from __fortify_fail_abort, > which somewhat defeats the purpose of -fstack-protector. I agree. This looks good to me. -- Cheers, Carlos.