Message ID | 20170719185036.GA32763@gmail.com |
---|---|
State | New |
Headers | show |
On Wed, Jul 19, 2017 at 11:50 AM, H.J. Lu <hongjiu.lu@intel.com> 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] ?: "<unknown>"); > + msg, > + (need_backtrace && __libc_argv[0] != NULL > + ? __libc_argv[0] : "<unknown>")); > } > > 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.
On 07/24/2017 08:50 AM, H.J. Lu wrote: > On Wed, Jul 19, 2017 at 11:50 AM, H.J. Lu <hongjiu.lu@intel.com> 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] ?: "<unknown>"); >> + msg, >> + (need_backtrace && __libc_argv[0] != NULL >> + ? __libc_argv[0] : "<unknown>")); >> } >> >> 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.
* H. J. Lu: > Glibc should avoid accessing corrupted stack from __fortify_fail_abort, > which somewhat defeats the purpose of -fstack-protector. Patch is reasonable (but more work on bug 21752 is unfortunately needed).
On Mon, Jul 24, 2017 at 10:07 AM, Florian Weimer <fw@deneb.enyo.de> wrote: > * H. J. Lu: > >> Glibc should avoid accessing corrupted stack from __fortify_fail_abort, >> which somewhat defeats the purpose of -fstack-protector. > > Patch is reasonable (but more work on bug 21752 is unfortunately > needed). That is true. It is only the first step. That is why I didn't close it.
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] ?: "<unknown>"); + msg, + (need_backtrace && __libc_argv[0] != NULL + ? __libc_argv[0] : "<unknown>")); } 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;