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

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

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