diff mbox series

elf: Add __libc_get_static_tls_bounds [BZ #16291]

Message ID 20210925004227.1829014-1-maskray@google.com
State New
Headers show
Series elf: Add __libc_get_static_tls_bounds [BZ #16291] | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Fangrui Song Sept. 25, 2021, 12:42 a.m. UTC
Sanitizer runtimes need static TLS boundaries for a variety of use cases.

* asan/hwasan/msan/tsan need to unpoison static TLS blocks to prevent false
  positives due to reusing the TLS blocks with a previous thread.
* lsan needs TCB for pointers into pthread_setspecific regions.

See https://maskray.me/blog/2021-02-14-all-about-thread-local-storage
for details.

compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp GetTls has
to infer the static TLS bounds from TP, _dl_get_tls_static_info, and
hard-coded TCB sizes. Currently this is somewhat robust for
aarch64/powerpc64/x86-64 but is brittle for many other architectures.

This patch implements __libc_get_static_tls_bounds@@GLIBC_PRIVATE which
is available in Android bionic since API level 31. This API allows the
sanitizer code to be more robust. _dl_get_tls_static_info@@GLIBC_PRIVATE
can probably be removed when Clang/GCC sanitizers drop reliance on it.
I am unclear whether the version should be GLIBC_2.*.

Tested that on x86_64-linux-gnu, __libc_get_static_tls_bounds output
matches sanitizer's GetTls.
---
 csu/libc-tls.c             |  2 ++
 elf/Versions               |  3 +++
 elf/dl-tls.c               | 13 +++++++++++++
 sysdeps/generic/ldsodefs.h |  6 ++++++
 4 files changed, 24 insertions(+)

Comments

Florian Weimer Sept. 27, 2021, 1:02 p.m. UTC | #1
* Fangrui Song:

> Sanitizer runtimes need static TLS boundaries for a variety of use cases.
>
> * asan/hwasan/msan/tsan need to unpoison static TLS blocks to prevent false
>   positives due to reusing the TLS blocks with a previous thread.
> * lsan needs TCB for pointers into pthread_setspecific regions.
>
> See https://maskray.me/blog/2021-02-14-all-about-thread-local-storage
> for details.
>
> compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp GetTls has
> to infer the static TLS bounds from TP, _dl_get_tls_static_info, and
> hard-coded TCB sizes. Currently this is somewhat robust for
> aarch64/powerpc64/x86-64 but is brittle for many other architectures.
>
> This patch implements __libc_get_static_tls_bounds@@GLIBC_PRIVATE which
> is available in Android bionic since API level 31. This API allows the
> sanitizer code to be more robust. _dl_get_tls_static_info@@GLIBC_PRIVATE
> can probably be removed when Clang/GCC sanitizers drop reliance on it.
> I am unclear whether the version should be GLIBC_2.*.

Does this really cover the right memory region?  I assume LSAN needs
something that identifies pointers to malloc'ed memory that are stored
in non-malloc'ed (mmap'ed) memory.  The static TLS region is certainly a
place where such pointers can be stored.  But struct pthread also
contains other such pointers: the DTV, the TPP data, and POSIX TLS
(pthread_setspecific) data, and struct pthread is not obviously part of
the static TLS region.

Thanks,
Florian
Joseph Myers Sept. 27, 2021, 5:38 p.m. UTC | #2
On Fri, 24 Sep 2021, Fangrui Song via Libc-alpha wrote:

> This patch implements __libc_get_static_tls_bounds@@GLIBC_PRIVATE which
> is available in Android bionic since API level 31. This API allows the
> sanitizer code to be more robust. _dl_get_tls_static_info@@GLIBC_PRIVATE
> can probably be removed when Clang/GCC sanitizers drop reliance on it.
> I am unclear whether the version should be GLIBC_2.*.

Any interface intended to be used outside of code distributed with glibc 
should use a symbol version GLIBC_2.* not GLIBC_PRIVATE.
Florian Weimer Sept. 27, 2021, 5:47 p.m. UTC | #3
* Joseph Myers:

> On Fri, 24 Sep 2021, Fangrui Song via Libc-alpha wrote:
>
>> This patch implements __libc_get_static_tls_bounds@@GLIBC_PRIVATE which
>> is available in Android bionic since API level 31. This API allows the
>> sanitizer code to be more robust. _dl_get_tls_static_info@@GLIBC_PRIVATE
>> can probably be removed when Clang/GCC sanitizers drop reliance on it.
>> I am unclear whether the version should be GLIBC_2.*.
>
> Any interface intended to be used outside of code distributed with glibc 
> should use a symbol version GLIBC_2.* not GLIBC_PRIVATE.

This interface will not be supportable indefinitely, though.  It's still
extremely tied to glibc implementation details.  So making it a public
symbol doesn't seem the right choice at this point.

Thanks,
Florian
Fangrui Song Sept. 27, 2021, 5:59 p.m. UTC | #4
On 2021-09-27, Florian Weimer wrote:
>* Fangrui Song:
>
>> Sanitizer runtimes need static TLS boundaries for a variety of use cases.
>>
>> * asan/hwasan/msan/tsan need to unpoison static TLS blocks to prevent false
>>   positives due to reusing the TLS blocks with a previous thread.
>> * lsan needs TCB for pointers into pthread_setspecific regions.
>>
>> See https://maskray.me/blog/2021-02-14-all-about-thread-local-storage
>> for details.
>>
>> compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp GetTls has
>> to infer the static TLS bounds from TP, _dl_get_tls_static_info, and
>> hard-coded TCB sizes. Currently this is somewhat robust for
>> aarch64/powerpc64/x86-64 but is brittle for many other architectures.
>>
>> This patch implements __libc_get_static_tls_bounds@@GLIBC_PRIVATE which
>> is available in Android bionic since API level 31. This API allows the
>> sanitizer code to be more robust. _dl_get_tls_static_info@@GLIBC_PRIVATE
>> can probably be removed when Clang/GCC sanitizers drop reliance on it.
>> I am unclear whether the version should be GLIBC_2.*.
>
>Does this really cover the right memory region?  I assume LSAN needs
>something that identifies pointers to malloc'ed memory that are stored
>in non-malloc'ed (mmap'ed) memory.  The static TLS region is certainly a
>place where such pointers can be stored.  But struct pthread also
>contains other such pointers: the DTV, the TPP data, and POSIX TLS
>(pthread_setspecific) data, and struct pthread is not obviously part of
>the static TLS region.

I know the pthread_setspecific leak detection is brittle but it is
currently implemented this way ;-)

https://maskray.me/blog/2021-02-14-all-about-thread-local-storage says

"On glibc, GetTls returned range includes
pthread::{specific_1stblock,specific} for thread-specific data keys.
There is currently a hack to ignore allocations from ld.so allocated
dynamic TLS blocks. Note: if the pthread::{specific_1stblock,specific}
pointers are encrypted, lsan cannot track the allocation."

If pthread::{specific_1stblock,specific} use an XOR technique (like
__cxa_atexit/setjmp) the pthread_setspecific leak detection will stop
working :(

---

In any case, the pthread_setspecific leak detection is a relatively
minor issue. The big issue is asan/msan/tsan false positives due to
reusing an (exited) thread stack or its TLS blocks.

Around
https://code.woboq.org/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp.html#435
there is very long messy code hard coding the thread descriptor size in
glibc.

Android `__libc_get_static_tls_bounds(&start_addr, &end_addr);` is the
most robust one.

---

I ported sanitizers to musl (https://reviews.llvm.org/D93848)
in LLVM 12.0.0 and fixed some TLS block detection aarch64/ppc64 issues
(https://reviews.llvm.org/D98926 and its follow-up, due to the
complexity I couldn't get it right in the first place), so I have some
understanding about sanitizers' TLS usage.
Fangrui Song Oct. 6, 2021, 8:23 p.m. UTC | #5
On 2021-09-27, Fangrui Song wrote:
>On 2021-09-27, Florian Weimer wrote:
>>* Fangrui Song:
>>
>>>Sanitizer runtimes need static TLS boundaries for a variety of use cases.
>>>
>>>* asan/hwasan/msan/tsan need to unpoison static TLS blocks to prevent false
>>>  positives due to reusing the TLS blocks with a previous thread.
>>>* lsan needs TCB for pointers into pthread_setspecific regions.
>>>
>>>See https://maskray.me/blog/2021-02-14-all-about-thread-local-storage
>>>for details.
>>>
>>>compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp GetTls has
>>>to infer the static TLS bounds from TP, _dl_get_tls_static_info, and
>>>hard-coded TCB sizes. Currently this is somewhat robust for
>>>aarch64/powerpc64/x86-64 but is brittle for many other architectures.
>>>
>>>This patch implements __libc_get_static_tls_bounds@@GLIBC_PRIVATE which
>>>is available in Android bionic since API level 31. This API allows the
>>>sanitizer code to be more robust. _dl_get_tls_static_info@@GLIBC_PRIVATE
>>>can probably be removed when Clang/GCC sanitizers drop reliance on it.
>>>I am unclear whether the version should be GLIBC_2.*.
>>
>>Does this really cover the right memory region?  I assume LSAN needs
>>something that identifies pointers to malloc'ed memory that are stored
>>in non-malloc'ed (mmap'ed) memory.  The static TLS region is certainly a
>>place where such pointers can be stored.  But struct pthread also
>>contains other such pointers: the DTV, the TPP data, and POSIX TLS
>>(pthread_setspecific) data, and struct pthread is not obviously part of
>>the static TLS region.
>
>I know the pthread_setspecific leak detection is brittle but it is
>currently implemented this way ;-)
>
>https://maskray.me/blog/2021-02-14-all-about-thread-local-storage says
>
>"On glibc, GetTls returned range includes
>pthread::{specific_1stblock,specific} for thread-specific data keys.
>There is currently a hack to ignore allocations from ld.so allocated
>dynamic TLS blocks. Note: if the pthread::{specific_1stblock,specific}
>pointers are encrypted, lsan cannot track the allocation."
>
>If pthread::{specific_1stblock,specific} use an XOR technique (like
>__cxa_atexit/setjmp) the pthread_setspecific leak detection will stop
>working :(
>
>---
>
>In any case, the pthread_setspecific leak detection is a relatively
>minor issue. The big issue is asan/msan/tsan false positives due to
>reusing an (exited) thread stack or its TLS blocks.
>
>Around
>https://code.woboq.org/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp.html#435
>there is very long messy code hard coding the thread descriptor size in
>glibc.
>
>Android `__libc_get_static_tls_bounds(&start_addr, &end_addr);` is the
>most robust one.
>
>---
>
>I ported sanitizers to musl (https://reviews.llvm.org/D93848)
>in LLVM 12.0.0 and fixed some TLS block detection aarch64/ppc64 issues
>(https://reviews.llvm.org/D98926 and its follow-up, due to the
>complexity I couldn't get it right in the first place), so I have some
>understanding about sanitizers' TLS usage.

Adhemerval showed me that the __libc_get_static_tls_bounds behavior is
expected on aarch64 as well (
__libc_get_static_tls_bounds should match sanitizer GetTls)

 From https://gist.github.com/MaskRay/e035b85dce008f0c6d4997b98354d355
```
$ ./testrun.sh ./test-tls-boundary
+++GetTls: 0x7f9c5fd6c000 4416
get_tls=0x7f9c600b4050
_dl_get_tls_static_info: 4416 64
get_static=0x7f9c600b4070
__libc_get_static_tls_bounds: 0x7f9c5fd6c000 4416
```



Is there any concern adding the interface?
Fangrui Song Oct. 15, 2021, 12:13 a.m. UTC | #6
On 2021-10-06, Fangrui Song wrote:
>On 2021-09-27, Fangrui Song wrote:
>>On 2021-09-27, Florian Weimer wrote:
>>>* Fangrui Song:
>>>
>>>>Sanitizer runtimes need static TLS boundaries for a variety of use cases.
>>>>
>>>>* asan/hwasan/msan/tsan need to unpoison static TLS blocks to prevent false
>>>> positives due to reusing the TLS blocks with a previous thread.
>>>>* lsan needs TCB for pointers into pthread_setspecific regions.
>>>>
>>>>See https://maskray.me/blog/2021-02-14-all-about-thread-local-storage
>>>>for details.
>>>>
>>>>compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp GetTls has
>>>>to infer the static TLS bounds from TP, _dl_get_tls_static_info, and
>>>>hard-coded TCB sizes. Currently this is somewhat robust for
>>>>aarch64/powerpc64/x86-64 but is brittle for many other architectures.
>>>>
>>>>This patch implements __libc_get_static_tls_bounds@@GLIBC_PRIVATE which
>>>>is available in Android bionic since API level 31. This API allows the
>>>>sanitizer code to be more robust. _dl_get_tls_static_info@@GLIBC_PRIVATE
>>>>can probably be removed when Clang/GCC sanitizers drop reliance on it.
>>>>I am unclear whether the version should be GLIBC_2.*.
>>>
>>>Does this really cover the right memory region?  I assume LSAN needs
>>>something that identifies pointers to malloc'ed memory that are stored
>>>in non-malloc'ed (mmap'ed) memory.  The static TLS region is certainly a
>>>place where such pointers can be stored.  But struct pthread also
>>>contains other such pointers: the DTV, the TPP data, and POSIX TLS
>>>(pthread_setspecific) data, and struct pthread is not obviously part of
>>>the static TLS region.
>>
>>I know the pthread_setspecific leak detection is brittle but it is
>>currently implemented this way ;-)
>>
>>https://maskray.me/blog/2021-02-14-all-about-thread-local-storage says
>>
>>"On glibc, GetTls returned range includes
>>pthread::{specific_1stblock,specific} for thread-specific data keys.
>>There is currently a hack to ignore allocations from ld.so allocated
>>dynamic TLS blocks. Note: if the pthread::{specific_1stblock,specific}
>>pointers are encrypted, lsan cannot track the allocation."
>>
>>If pthread::{specific_1stblock,specific} use an XOR technique (like
>>__cxa_atexit/setjmp) the pthread_setspecific leak detection will stop
>>working :(
>>
>>---
>>
>>In any case, the pthread_setspecific leak detection is a relatively
>>minor issue. The big issue is asan/msan/tsan false positives due to
>>reusing an (exited) thread stack or its TLS blocks.
>>
>>Around
>>https://code.woboq.org/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp.html#435
>>there is very long messy code hard coding the thread descriptor size in
>>glibc.
>>
>>Android `__libc_get_static_tls_bounds(&start_addr, &end_addr);` is the
>>most robust one.
>>
>>---
>>
>>I ported sanitizers to musl (https://reviews.llvm.org/D93848)
>>in LLVM 12.0.0 and fixed some TLS block detection aarch64/ppc64 issues
>>(https://reviews.llvm.org/D98926 and its follow-up, due to the
>>complexity I couldn't get it right in the first place), so I have some
>>understanding about sanitizers' TLS usage.
>
>Adhemerval showed me that the __libc_get_static_tls_bounds behavior is
>expected on aarch64 as well (
>__libc_get_static_tls_bounds should match sanitizer GetTls)
>
>From https://gist.github.com/MaskRay/e035b85dce008f0c6d4997b98354d355
>```
>$ ./testrun.sh ./test-tls-boundary
>+++GetTls: 0x7f9c5fd6c000 4416
>get_tls=0x7f9c600b4050
>_dl_get_tls_static_info: 4416 64
>get_static=0x7f9c600b4070
>__libc_get_static_tls_bounds: 0x7f9c5fd6c000 4416
>```
>
>
>
>Is there any concern adding the interface?

Gentle ping...
diff mbox series

Patch

diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index 5515204863..433c9bfcc6 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -46,6 +46,8 @@  bool _dl_tls_dtv_gaps;
 struct dtv_slotinfo_list *_dl_tls_dtv_slotinfo_list;
 /* Number of modules in the static TLS block.  */
 size_t _dl_tls_static_nelem;
+/* Start of the static TLS block.  */
+void *_dl_tls_static_start;
 /* Size of the static TLS block.  */
 size_t _dl_tls_static_size;
 /* Size actually allocated in the static TLS block.  */
diff --git a/elf/Versions b/elf/Versions
index 775aab62af..ec095022f9 100644
--- a/elf/Versions
+++ b/elf/Versions
@@ -72,5 +72,8 @@  ld {
 
     # Set value of a tunable.
     __tunable_get_val;
+
+    # Used by sanitizers.
+    __libc_get_static_tls_bounds;
   }
 }
diff --git a/elf/dl-tls.c b/elf/dl-tls.c
index d554ae4497..49edb0e452 100644
--- a/elf/dl-tls.c
+++ b/elf/dl-tls.c
@@ -400,6 +400,15 @@  _dl_get_tls_static_info (size_t *sizep, size_t *alignp)
   *alignp = GLRO (dl_tls_static_align);
 }
 
+/* Get start and stop addresses of the static TLS block.  This
+   function will be used by Clang and GCC sanitizers.  */
+void
+__libc_get_static_tls_bounds (void **startp, void **endp)
+{
+  *startp = GLRO (dl_tls_static_start);
+  *endp = GLRO (dl_tls_static_start) + GLRO (dl_tls_static_size);
+}
+
 /* Derive the location of the pointer to the start of the original
    allocation (before alignment) from the pointer to the TCB.  */
 static inline void **
@@ -448,6 +457,8 @@  _dl_allocate_tls_storage (void)
   /* Clear the TCB data structure.  We can't ask the caller (i.e.
      libpthread) to do it, because we will initialize the DTV et al.  */
   memset (result, '\0', TLS_TCB_SIZE);
+
+  GLRO (dl_tls_static_start) = aligned;
 #elif TLS_DTV_AT_TP
   /* Pre-TCB and TCB come before the TLS blocks.  The layout computed
      in _dl_determine_tlsoffset assumes that the TCB is aligned to the
@@ -462,6 +473,8 @@  _dl_allocate_tls_storage (void)
      it.  We can't ask the caller (i.e. libpthread) to do it, because
      we will initialize the DTV et al.  */
   memset (result - TLS_PRE_TCB_SIZE, '\0', TLS_PRE_TCB_SIZE + TLS_TCB_SIZE);
+
+  GLRO (dl_tls_static_start) = result - TLS_PRE_TCB_SIZE;
 #endif
 
   /* Record the value of the original pointer for later
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index d49529da0d..ad4672da26 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -609,6 +609,9 @@  struct rtld_global_ro
      binaries, don't honor for PIEs).  */
   EXTERN ElfW(Addr) _dl_use_load_bias;
 
+  /* Start of the static TLS block.  */
+  EXTERN void *_dl_tls_static_start;
+
   /* Size of the static TLS block.  */
   EXTERN size_t _dl_tls_static_size;
 
@@ -1229,6 +1232,9 @@  rtld_hidden_proto (_dl_allocate_tls)
 /* Get size and alignment requirements of the static TLS block.  */
 extern void _dl_get_tls_static_info (size_t *sizep, size_t *alignp);
 
+/* Get start and end addresses of the static TLS block.  */
+extern void __libc_get_static_tls_bounds (void **startp, void **endp);
+
 extern void _dl_allocate_static_tls (struct link_map *map) attribute_hidden;
 
 /* These are internal entry points to the two halves of _dl_allocate_tls,