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

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