[PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer.

[PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer.

[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

Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer.

[Florian Weimer]

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

Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer.

[Mark Wielaard]

[-- 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

Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer.

[Roland McGrath]

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

Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer.

[Mark Wielaard]

[-- 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

Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer.

[Roland McGrath]

That looks perfect to me.  But Allan has to approve during the freeze.

Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer.

[Allan McRae]

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

Re: [PATCH] i386 TLS_INIT_TP might produce bogus asm changing stack pointer.

[Florian Weimer]

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