[PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

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

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(+)

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,
-- 
2.33.0.685.g46640cef36-goog

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Florian Weimer]

* 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

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Fangrui Song]

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.

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Fangrui Song]

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?

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Fangrui Song]

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

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Fāng-ruì Sòng]

On Thu, Oct 14, 2021 at 5:13 PM Fangrui Song <maskray@google.com> wrote:
>
> 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...


CC gcc-patches which ports compiler-rt and may be interested in more
reliable sanitizers.

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Fāng-ruì Sòng]

On Tue, Oct 19, 2021 at 12:37 PM Fāng-ruì Sòng <maskray@google.com> wrote:
>
> On Thu, Oct 14, 2021 at 5:13 PM Fangrui Song <maskray@google.com> wrote:
> >
> > 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...
>
>
> CC gcc-patches which ports compiler-rt and may be interested in more
> reliable sanitizers.

PING^3

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Florian Weimer]

* Fāng-ruì Sòng:

> PING^3

I think the core issue with this patch is like this:

* I do not want to commit glibc to a public API that disallows future
  changes to the way we allocate static TLS.  While static TLS objects
  cannot move in memory, the extent of the static TLS area (minimum and
  maximum address) is not fixed by ABI necessities.

* Joseph does not want to add a GLIBC_PRIVATE ABI that is exclusively
  used externally.

I have tried repeatly to wrap my head around how the sanitizers use the
static TLS boundary information.  Based on what I can tell, there are
several applications:

1. Thead Sanitizer needs to know application-accessible thread-specific
   memory that is carried via the glibc thread (stack) cache from one
   thread to another one, seemingly without synchronization.  Since the
   synchronization happens internally within glibc, without that extra
   knowledge, Thread Sanitizer would report a false positive.  This
   covers only data to which the application has direct access, internal
   access by glibc does not count because it is not going to be
   instrumented.

2. Address Sanitizer needs TLS boundary information for bounds checking.
   Again this only applies to accesses that can be instrumented, so only
   objects whose addresses leak to application code count.  (Maybe this
   is a fringe use case, though: it seems to apply only to “extern
   __thread int a[];“ and similar declarations, where the declared type
   is not enough.)

3. Leak Sanitizer needs to find all per-thread pointers pointing into
   the malloc heap, within memory not itself allocated by malloc.  This
   includes the DTV, pthread_getspecific metadata, and perhaps also user
   data set by pthread_getspecific, and of course static TLS variables.
   This goes someone beyond what people would usually consider static
   TLS: the DTV and struct pthread are not really part of static TLS as
   such.  And struct pthread has several members that contain malloc'ed
   pointers.

Is this a complete list of uses?

I *think* you can get what you need via existing GLIBC_PRIVATE
interfaces.  But in order to describe how to caox the data out of glibc,
I need to know what you need.

(Cc:ing a few people from a recent GCC thread.)

Thanks,
Florian

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Fāng-ruì Sòng]

On Mon, Nov 29, 2021 at 11:30 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Fāng-ruì Sòng:
>
> > PING^3
>
> I think the core issue with this patch is like this:
>
> * I do not want to commit glibc to a public API that disallows future
>   changes to the way we allocate static TLS.  While static TLS objects
>   cannot move in memory, the extent of the static TLS area (minimum and
>   maximum address) is not fixed by ABI necessities.
>
> * Joseph does not want to add a GLIBC_PRIVATE ABI that is exclusively
>   used externally.
>
> I have tried repeatly to wrap my head around how the sanitizers use the
> static TLS boundary information.  Based on what I can tell, there are
> several applications:
>
> 1. Thead Sanitizer needs to know application-accessible thread-specific
>    memory that is carried via the glibc thread (stack) cache from one
>    thread to another one, seemingly without synchronization.  Since the
>    synchronization happens internally within glibc, without that extra
>    knowledge, Thread Sanitizer would report a false positive.  This
>    covers only data to which the application has direct access, internal
>    access by glibc does not count because it is not going to be
>    instrumented.
>
> 2. Address Sanitizer needs TLS boundary information for bounds checking.
>    Again this only applies to accesses that can be instrumented, so only
>    objects whose addresses leak to application code count.  (Maybe this
>    is a fringe use case, though: it seems to apply only to “extern
>    __thread int a[];“ and similar declarations, where the declared type
>    is not enough.)
>
> 3. Leak Sanitizer needs to find all per-thread pointers pointing into
>    the malloc heap, within memory not itself allocated by malloc.  This
>    includes the DTV, pthread_getspecific metadata, and perhaps also user
>    data set by pthread_getspecific, and of course static TLS variables.
>    This goes someone beyond what people would usually consider static
>    TLS: the DTV and struct pthread are not really part of static TLS as
>    such.  And struct pthread has several members that contain malloc'ed
>    pointers.
>
> Is this a complete list of uses?

Perhaps add HWAddressSanitizer along with Address Sanitizer and Memory
Sanitizer along with Thread Sanitizer.
Then I think the list is complete.

> I *think* you can get what you need via existing GLIBC_PRIVATE
> interfaces.  But in order to describe how to caox the data out of glibc,
> I need to know what you need.

Unfortunate no, not reliably. Currently _dl_get_tls_static_info
(https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp#L207)
is used. It has the size information but not the start address, so the
sanitizer runtime is doing very ugly/error-prone probing of the start
address by reading the thread pointer and hard coding the `struct
pthread` size for various glibc versions.
Every time `struct pthreads` increases in glibc, sanitizer runtime has to adapt.

__libc_get_static_tls_bounds if supported by glibc, can replace
_dl_get_tls_static_info and the existing glibc specific GetTls hacks,
and provide a bit more compatibility when glibc struct pthreads
increases.

> (Cc:ing a few people from a recent GCC thread.)
>
> Thanks,
> Florian
>


-- 
宋方睿

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Florian Weimer]

* Fāng-ruì Sòng:

>> I *think* you can get what you need via existing GLIBC_PRIVATE
>> interfaces.  But in order to describe how to caox the data out of glibc,
>> I need to know what you need.
>
> Unfortunate no, not reliably. Currently _dl_get_tls_static_info
> (https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp#L207)
> is used. It has the size information but not the start address, so the
> sanitizer runtime is doing very ugly/error-prone probing of the start
> address by reading the thread pointer and hard coding the `struct
> pthread` size for various glibc versions.
> Every time `struct pthreads` increases in glibc, sanitizer runtime has to adapt.
>
> __libc_get_static_tls_bounds if supported by glibc, can replace
> _dl_get_tls_static_info and the existing glibc specific GetTls hacks,
> and provide a bit more compatibility when glibc struct pthreads
> increases.

We already export the size of struct pthread, though, as a GLIBC_PRIVATE
symbol.

  const unsigned int *psize = dlvsym("_thread_db_sizeof_pthread",
                                     "GLIBC_PRIVATE");
  if (psize != nullptr) {
    val = *psize;
    atomic_store_relaxed(&thread_descriptor_size, val);
    return val;
  }

in ThreadDescriptorSize() in
compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp should work
with any further glibc changes today.  It definitely beats hard-coding
the architecture-specific struct pthread size.

Using _thread_db_sizeof_pthread does not remove all magic constants from
the code, and there is of course the _dl_get_tls_static_info symbol
usage.

This is a fairly recent change (glibc 2.34, I believe) that was
introduced to improve thread debugging without debuginfo present in the
glibc objects (which is why it's a GLIBC_PRIVATE symbol).  libthread_db
is bundled with glibc, so the _thread_db_* symbols are not gurantueed to
be stable, but there are no immediate plans to change this interface.

Thanks,
Florian

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

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

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] elf: Add __libc_get_static_tls_bounds [BZ #16291]

[Florian Weimer]

* 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