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

["Fāng-ruì Sòng" <maskray@google.com>]

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.