* [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer. @ 2014-08-26 8:27 Mark Wielaard 2014-08-26 8:34 ` Florian Weimer 0 siblings, 1 reply; 8+ messages in thread From: Mark Wielaard @ 2014-08-26 8:27 UTC (permalink / raw) To: libc-alpha; +Cc: Siddhesh Poyarekar, Florian Weimer, Mark Wielaard TLS_INIT_TP in sysdeps/i386/nptl/tls.h uses some hand written asm to generate a set_thread_area that might result in exchanging ebx and esp around the syscall causing introspection tools like valgrind to loose track of the user stack. Just use INTERNAL_SYSCALL which makes sure esp isn't changed arbitrarily. Before the patch the code would generate: mov $0xf3,%eax movl $0xfffff,0x8(%esp) movl $0x51,0xc(%esp) xchg %esp,%ebx int $0x80 xchg %esp,%ebx Using INTERNAL_SYSCALL instead will generate: movl $0xfffff,0x8(%esp) movl $0x51,0xc(%esp) xchg %ecx,%ebx mov $0xf3,%eax int $0x80 xchg %ecx,%ebx Thanks to Florian Weimer for analysing why the original code generated the bogus esp usage: _segdescr.desc happens to be at the top of the stack, so its address is in %esp. The asm statement says that %3 is an input, so its value will not change, and GCC can use %esp as the input register for the expression &_segdescr.desc. But the constraints do not fully describe the asm statement because the %3 register is actually modified, albeit only temporarily. https://bugzilla.redhat.com/show_bug.cgi?id=1133134 * sysdeps/i386/nptl/tls.h (TLS_INIT_TP): Use INTERNAL_SYSCALL to call set_thread_area instead of hand written asm. --- ChangeLog | 5 +++++ sysdeps/i386/nptl/tls.h | 8 ++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/sysdeps/i386/nptl/tls.h b/sysdeps/i386/nptl/tls.h index ac9c9a2..052ea64 100644 --- a/sysdeps/i386/nptl/tls.h +++ b/sysdeps/i386/nptl/tls.h @@ -231,12 +231,8 @@ tls_fill_user_desc (union user_desc_init *desc, tls_fill_user_desc (&_segdescr, -1, _thrdescr); \ \ /* Install the TLS. */ \ - asm volatile (TLS_LOAD_EBX \ - "int $0x80\n\t" \ - TLS_LOAD_EBX \ - : "=a" (_result), "=m" (_segdescr.desc.entry_number) \ - : "0" (__NR_set_thread_area), \ - TLS_EBX_ARG (&_segdescr.desc), "m" (_segdescr.desc)); \ + INTERNAL_SYSCALL_DECL (err); \ + _result = INTERNAL_SYSCALL (set_thread_area, err, 1, &_segdescr.desc); \ \ if (_result == 0) \ /* We know the index in the GDT, now load the segment register. \ -- 1.9.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer. 2014-08-26 8:27 [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer Mark Wielaard @ 2014-08-26 8:34 ` Florian Weimer 2014-08-26 12:27 ` Mark Wielaard 0 siblings, 1 reply; 8+ messages in thread From: Florian Weimer @ 2014-08-26 8:34 UTC (permalink / raw) To: Mark Wielaard, libc-alpha; +Cc: Siddhesh Poyarekar On 08/26/2014 10:27 AM, Mark Wielaard wrote: > * sysdeps/i386/nptl/tls.h (TLS_INIT_TP): Use INTERNAL_SYSCALL > to call set_thread_area instead of hand written asm. Patch looks good, but you can remove these #defines now: # ifdef __PIC__ # define TLS_EBX_ARG "r" # define TLS_LOAD_EBX "xchgl %3, %%ebx\n\t" # else # define TLS_EBX_ARG "b" # define TLS_LOAD_EBX # endif (INTERNAL_SYSCALL is not affected by this issue because it uses explicit register constraints, and not the general "r" constraint.) -- Florian Weimer / Red Hat Product Security ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer. 2014-08-26 8:34 ` Florian Weimer @ 2014-08-26 12:27 ` Mark Wielaard 2014-08-26 21:29 ` Roland McGrath 0 siblings, 1 reply; 8+ messages in thread From: Mark Wielaard @ 2014-08-26 12:27 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Siddhesh Poyarekar [-- Attachment #1: Type: text/plain, Size: 832 bytes --] On Tue, 2014-08-26 at 10:34 +0200, Florian Weimer wrote: > On 08/26/2014 10:27 AM, Mark Wielaard wrote: > > * sysdeps/i386/nptl/tls.h (TLS_INIT_TP): Use INTERNAL_SYSCALL > > to call set_thread_area instead of hand written asm. > > Patch looks good, but you can remove these #defines now: > > # ifdef __PIC__ > # define TLS_EBX_ARG "r" > # define TLS_LOAD_EBX "xchgl %3, %%ebx\n\t" > # else > # define TLS_EBX_ARG "b" > # define TLS_LOAD_EBX > # endif > > (INTERNAL_SYSCALL is not affected by this issue because it uses explicit > register constraints, and not the general "r" constraint.) Yes, you are right. There were some more defines around that block that are also no longer used. I removed them all (and made sure everything still builds of course). Updated commit attached. Thanks, Mark [-- Attachment #2: 0001-i386-TLS_INIT_TP-might-produce-bogus-asm-changing-st.patch --] [-- Type: text/x-patch, Size: 4081 bytes --] From f8ce6debd1aea3265f7e8bb1ea898d263ac7c76f Mon Sep 17 00:00:00 2001 From: Mark Wielaard <mjw@redhat.com> Date: Tue, 26 Aug 2014 09:57:56 +0200 Subject: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer. TLS_INIT_TP in sysdeps/i386/nptl/tls.h uses some hand written asm to generate a set_thread_area that might result in exchanging ebx and esp around the syscall causing introspection tools like valgrind to loose track of the user stack. Just use INTERNAL_SYSCALL which makes sure esp isn't changed arbitrarily. Before the patch the code would generate: mov $0xf3,%eax movl $0xfffff,0x8(%esp) movl $0x51,0xc(%esp) xchg %esp,%ebx int $0x80 xchg %esp,%ebx Using INTERNAL_SYSCALL instead will generate: movl $0xfffff,0x8(%esp) movl $0x51,0xc(%esp) xchg %ecx,%ebx mov $0xf3,%eax int $0x80 xchg %ecx,%ebx Thanks to Florian Weimer for analysing why the original code generated the bogus esp usage: _segdescr.desc happens to be at the top of the stack, so its address is in %esp. The asm statement says that %3 is an input, so its value will not change, and GCC can use %esp as the input register for the expression &_segdescr.desc. But the constraints do not fully describe the asm statement because the %3 register is actually modified, albeit only temporarily. https://bugzilla.redhat.com/show_bug.cgi?id=1133134 * sysdeps/i386/nptl/tls.h (TLS_INIT_TP): Use INTERNAL_SYSCALL to call set_thread_area instead of hand written asm. (__NR_set_thread_area): Removed define. (TLS_FLAG_WRITABLE): Likewise. (__ASSUME_SET_THREAD_AREA): Remove check. (TLS_EBX_ARG): Remove define. (TLS_LOAD_EBX): Likewise. --- ChangeLog | 10 ++++++++++ sysdeps/i386/nptl/tls.h | 31 ++----------------------------- 2 files changed, 12 insertions(+), 29 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4d2eb52..2eefb33 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2014-08-26 Mark Wielaard <mjw@redhat.com> + + * sysdeps/i386/nptl/tls.h (TLS_INIT_TP): Use INTERNAL_SYSCALL + to call set_thread_area instead of hand written asm. + (__NR_set_thread_area): Removed define. + (TLS_FLAG_WRITABLE): Likewise. + (__ASSUME_SET_THREAD_AREA): Remove check. + (TLS_EBX_ARG): Remove define. + (TLS_LOAD_EBX): Likewise. + 2014-08-21 Siddhesh Poyarekar <siddhesh@redhat.com> * nptl/Makefile (CFLAGS-pthread_atfork.c): Remove. diff --git a/sysdeps/i386/nptl/tls.h b/sysdeps/i386/nptl/tls.h index ac9c9a2..d7302ba 100644 --- a/sysdeps/i386/nptl/tls.h +++ b/sysdeps/i386/nptl/tls.h @@ -154,29 +154,6 @@ union user_desc_init __asm ("movw %w0, %%gs" :: "q" (val)) # endif - -# ifndef __NR_set_thread_area -# define __NR_set_thread_area 243 -# endif -# ifndef TLS_FLAG_WRITABLE -# define TLS_FLAG_WRITABLE 0x00000001 -# endif - -// XXX Enable for the real world. -#if 0 -# ifndef __ASSUME_SET_THREAD_AREA -# error "we need set_thread_area" -# endif -#endif - -# ifdef __PIC__ -# define TLS_EBX_ARG "r" -# define TLS_LOAD_EBX "xchgl %3, %%ebx\n\t" -# else -# define TLS_EBX_ARG "b" -# define TLS_LOAD_EBX -# endif - #if defined NEED_DL_SYSINFO # define INIT_SYSINFO \ _head->sysinfo = GLRO(dl_sysinfo) @@ -231,12 +208,8 @@ tls_fill_user_desc (union user_desc_init *desc, tls_fill_user_desc (&_segdescr, -1, _thrdescr); \ \ /* Install the TLS. */ \ - asm volatile (TLS_LOAD_EBX \ - "int $0x80\n\t" \ - TLS_LOAD_EBX \ - : "=a" (_result), "=m" (_segdescr.desc.entry_number) \ - : "0" (__NR_set_thread_area), \ - TLS_EBX_ARG (&_segdescr.desc), "m" (_segdescr.desc)); \ + INTERNAL_SYSCALL_DECL (err); \ + _result = INTERNAL_SYSCALL (set_thread_area, err, 1, &_segdescr.desc); \ \ if (_result == 0) \ /* We know the index in the GDT, now load the segment register. \ -- 1.9.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer. 2014-08-26 12:27 ` Mark Wielaard @ 2014-08-26 21:29 ` Roland McGrath 2014-08-27 15:12 ` Mark Wielaard 0 siblings, 1 reply; 8+ messages in thread From: Roland McGrath @ 2014-08-26 21:29 UTC (permalink / raw) To: Mark Wielaard; +Cc: Florian Weimer, libc-alpha, Siddhesh Poyarekar The change looks good and even seems probably appropriate to slip in during the release freeze. But, per policy, as this has user-visible effects it needs a bugzilla item filed. Thanks, Roland ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer. 2014-08-26 21:29 ` Roland McGrath @ 2014-08-27 15:12 ` Mark Wielaard 2014-08-27 16:58 ` Roland McGrath 0 siblings, 1 reply; 8+ messages in thread From: Mark Wielaard @ 2014-08-27 15:12 UTC (permalink / raw) To: Roland McGrath; +Cc: Florian Weimer, libc-alpha, Siddhesh Poyarekar [-- Attachment #1: Type: text/plain, Size: 564 bytes --] On Tue, 2014-08-26 at 14:29 -0700, Roland McGrath wrote: > The change looks good and even seems probably appropriate to slip in during > the release freeze. That would be great, it does cause some head-scratching for anybody trying to run any i386 program under valgrind with glibc 2.20. > But, per policy, as this has user-visible effects it > needs a bugzilla item filed. Filed https://sourceware.org/bugzilla/show_bug.cgi?id=17319 Updated commit mentioning the bug number in the commit message and ChangeLog entry attached. Thanks, Mark [-- Attachment #2: 0001-i386-TLS_INIT_TP-might-produce-bogus-asm-changing-st.patch --] [-- Type: text/x-patch, Size: 4057 bytes --] From ab4888b207f07fd762c485f94f9543b0ed456a4d Mon Sep 17 00:00:00 2001 From: Mark Wielaard <mjw@redhat.com> Date: Wed, 27 Aug 2014 17:07:58 +0200 Subject: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer [BZ #17319] TLS_INIT_TP in sysdeps/i386/nptl/tls.h uses some hand written asm to generate a set_thread_area that might result in exchanging ebx and esp around the syscall causing introspection tools like valgrind to loose track of the user stack. Just use INTERNAL_SYSCALL which makes sure esp isn't changed arbitrarily. Before the patch the code would generate: mov $0xf3,%eax movl $0xfffff,0x8(%esp) movl $0x51,0xc(%esp) xchg %esp,%ebx int $0x80 xchg %esp,%ebx Using INTERNAL_SYSCALL instead will generate: movl $0xfffff,0x8(%esp) movl $0x51,0xc(%esp) xchg %ecx,%ebx mov $0xf3,%eax int $0x80 xchg %ecx,%ebx Thanks to Florian Weimer for analysing why the original code generated the bogus esp usage: _segdescr.desc happens to be at the top of the stack, so its address is in %esp. The asm statement says that %3 is an input, so its value will not change, and GCC can use %esp as the input register for the expression &_segdescr.desc. But the constraints do not fully describe the asm statement because the %3 register is actually modified, albeit only temporarily. [BZ #17319] * sysdeps/i386/nptl/tls.h (TLS_INIT_TP): Use INTERNAL_SYSCALL to call set_thread_area instead of hand written asm. (__NR_set_thread_area): Removed define. (TLS_FLAG_WRITABLE): Likewise. (__ASSUME_SET_THREAD_AREA): Remove check. (TLS_EBX_ARG): Remove define. (TLS_LOAD_EBX): Likewise. --- ChangeLog | 11 +++++++++++ sysdeps/i386/nptl/tls.h | 31 ++----------------------------- 2 files changed, 13 insertions(+), 29 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6261ed4..ceeb06b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,14 @@ +2014-08-27 Mark Wielaard <mjw@redhat.com> + + [BZ #17319] + * sysdeps/i386/nptl/tls.h (TLS_INIT_TP): Use INTERNAL_SYSCALL + to call set_thread_area instead of hand written asm. + (__NR_set_thread_area): Removed define. + (TLS_FLAG_WRITABLE): Likewise. + (__ASSUME_SET_THREAD_AREA): Remove check. + (TLS_EBX_ARG): Remove define. + (TLS_LOAD_EBX): Likewise. + 2014-08-27 Allan McRae <allan@archlinux.org> * sysdeps/i386/fpu/libm-test-ulps: Update ULPs. diff --git a/sysdeps/i386/nptl/tls.h b/sysdeps/i386/nptl/tls.h index ac9c9a2..d7302ba 100644 --- a/sysdeps/i386/nptl/tls.h +++ b/sysdeps/i386/nptl/tls.h @@ -154,29 +154,6 @@ union user_desc_init __asm ("movw %w0, %%gs" :: "q" (val)) # endif - -# ifndef __NR_set_thread_area -# define __NR_set_thread_area 243 -# endif -# ifndef TLS_FLAG_WRITABLE -# define TLS_FLAG_WRITABLE 0x00000001 -# endif - -// XXX Enable for the real world. -#if 0 -# ifndef __ASSUME_SET_THREAD_AREA -# error "we need set_thread_area" -# endif -#endif - -# ifdef __PIC__ -# define TLS_EBX_ARG "r" -# define TLS_LOAD_EBX "xchgl %3, %%ebx\n\t" -# else -# define TLS_EBX_ARG "b" -# define TLS_LOAD_EBX -# endif - #if defined NEED_DL_SYSINFO # define INIT_SYSINFO \ _head->sysinfo = GLRO(dl_sysinfo) @@ -231,12 +208,8 @@ tls_fill_user_desc (union user_desc_init *desc, tls_fill_user_desc (&_segdescr, -1, _thrdescr); \ \ /* Install the TLS. */ \ - asm volatile (TLS_LOAD_EBX \ - "int $0x80\n\t" \ - TLS_LOAD_EBX \ - : "=a" (_result), "=m" (_segdescr.desc.entry_number) \ - : "0" (__NR_set_thread_area), \ - TLS_EBX_ARG (&_segdescr.desc), "m" (_segdescr.desc)); \ + INTERNAL_SYSCALL_DECL (err); \ + _result = INTERNAL_SYSCALL (set_thread_area, err, 1, &_segdescr.desc); \ \ if (_result == 0) \ /* We know the index in the GDT, now load the segment register. \ -- 1.9.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer. 2014-08-27 15:12 ` Mark Wielaard @ 2014-08-27 16:58 ` Roland McGrath 2014-08-27 23:01 ` Allan McRae 0 siblings, 1 reply; 8+ messages in thread From: Roland McGrath @ 2014-08-27 16:58 UTC (permalink / raw) To: Mark Wielaard; +Cc: Florian Weimer, libc-alpha, Siddhesh Poyarekar, Allan McRae That looks perfect to me. But Allan has to approve during the freeze. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer. 2014-08-27 16:58 ` Roland McGrath @ 2014-08-27 23:01 ` Allan McRae 2014-08-28 8:21 ` Florian Weimer 0 siblings, 1 reply; 8+ messages in thread From: Allan McRae @ 2014-08-27 23:01 UTC (permalink / raw) To: Roland McGrath, Mark Wielaard Cc: Florian Weimer, libc-alpha, Siddhesh Poyarekar On 28/08/14 02:58, Roland McGrath wrote: > That looks perfect to me. But Allan has to approve during the freeze. > OK for master. Allan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer. 2014-08-27 23:01 ` Allan McRae @ 2014-08-28 8:21 ` Florian Weimer 0 siblings, 0 replies; 8+ messages in thread From: Florian Weimer @ 2014-08-28 8:21 UTC (permalink / raw) To: Allan McRae, Roland McGrath, Mark Wielaard; +Cc: libc-alpha, Siddhesh Poyarekar On 08/28/2014 01:01 AM, Allan McRae wrote: > On 28/08/14 02:58, Roland McGrath wrote: >> That looks perfect to me. But Allan has to approve during the freeze. >> > > OK for master. Thanks. I added the bug number to NEWS and pushed this on behalf of Mark because he hasn't got commit access. -- Florian Weimer / Red Hat Product Security ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-08-28 8:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-26 8:27 [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer Mark Wielaard 2014-08-26 8:34 ` Florian Weimer 2014-08-26 12:27 ` Mark Wielaard 2014-08-26 21:29 ` Roland McGrath 2014-08-27 15:12 ` Mark Wielaard 2014-08-27 16:58 ` Roland McGrath 2014-08-27 23:01 ` Allan McRae 2014-08-28 8:21 ` Florian Weimer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).