From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vs1-xe2c.google.com (mail-vs1-xe2c.google.com [IPv6:2607:f8b0:4864:20::e2c]) by sourceware.org (Postfix) with ESMTPS id C3AAF3858C74 for ; Tue, 1 Feb 2022 13:35:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C3AAF3858C74 Received: by mail-vs1-xe2c.google.com with SMTP id f6so16075988vsa.5 for ; Tue, 01 Feb 2022 05:35:39 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=WZzLdNHogaXBYJf9/sFfyGIlTrO18FuQtU/yT2cAUxQ=; b=4B11pHsTc6oJEK+c4Zbp+15ciHvYwY5XgL0Q8+OO3RzjO1PqKM1jyrUEJPvxZMt6DQ juvHO3fLu6mIQEomP5M7Abo9wG2U72E2KifopvCrcAWj8OmVH63N+esV+IaHY94XVAOA 1Ci1poKiayVevoBe+rZIDWqstip3HpKXBG2eK0vYlSqXzkIL40NslpTTsfVM66SUIDEC uNc2J35J4LqPkbFySG+uUGJfbOl2G1b2iLzpMDrgsEtzt43A4c/FWfdTxX0r0Tf5QsP+ 9NZUJ1B/X1NZyRNo3uMf9iPUuj2/CRg8NxRk6u4E+q1P7dNZgl9A3uLZtgIHeyqgMkNp P4BA== X-Gm-Message-State: AOAM533fxHoGkI+dOu7Se3xVbRDpuhdwzBydi6oRctjbUeJCQoIseW4+ liigeTtElCvPzKNPt+gJ3jVHWg== X-Google-Smtp-Source: ABdhPJw38IWmGKpNp6ht2AXibzrfMjXNAUTBGU7kSSXDpunkCXZ1LvddujnFCJEaVN4+Gffi+JMsYQ== X-Received: by 2002:a67:efc2:: with SMTP id s2mr4433433vsp.32.1643722539091; Tue, 01 Feb 2022 05:35:39 -0800 (PST) Received: from ?IPV6:2804:431:c7ca:709a:f3cb:a92a:e1ce:d27d? ([2804:431:c7ca:709a:f3cb:a92a:e1ce:d27d]) by smtp.gmail.com with ESMTPSA id e17sm4201067vsl.21.2022.02.01.05.35.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 01 Feb 2022 05:35:38 -0800 (PST) Message-ID: <76182b38-d288-1cc5-5f09-2ad99a2509fb@linaro.org> Date: Tue, 1 Feb 2022 10:35:36 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.1 Subject: Re: [PATCH v12 2/4] elf: Fix initial-exec TLS access on audit modules (BZ #28096) Content-Language: en-US To: Carlos O'Donell , libc-alpha@sourceware.org, jma14 Cc: John Mellor-Crummey , Ben Woodard References: <20220125183700.1280931-1-adhemerval.zanella@linaro.org> <20220125183700.1280931-3-adhemerval.zanella@linaro.org> From: Adhemerval Zanella In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Feb 2022 13:35:42 -0000 On 01/02/2022 02:29, Carlos O'Donell wrote: > On 1/25/22 13:36, Adhemerval Zanella wrote: >> For audit modules and dependencies with initial-exec TLS, we can not >> set the initial TLS image on default loader initialization because it >> would already be set by the audit setup. However, subsequent thread >> creation would need to follow the default behaviour. > > Correct, there is a circular dependency, we need the initial TLS image setup for > the auditor before the loader does the setup. > >> This patch fixes it by setting l_auditing link_map field not only >> for the audit modules, but also for all its dependencies. This is >> used on _dl_allocate_tls_init to avoid the static TLS initialization >> only at loading time. > > s/only at loading/at load/g Ack. > >> Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu. > > Tested on x86_64 and i686 and it looks good for glibc 2.35. > > Please post v13 with fixes and I'll ACK that as release manager. > > >> --- >> elf/Makefile | 8 ++++ >> elf/dl-tls.c | 13 +++++-- >> elf/rtld.c | 2 +- >> elf/tst-audit21.c | 42 ++++++++++++++++++++ >> elf/tst-auditmod21a.c | 80 ++++++++++++++++++++++++++++++++++++++ >> elf/tst-auditmod21b.c | 22 +++++++++++ >> nptl/allocatestack.c | 2 +- >> sysdeps/generic/ldsodefs.h | 2 +- >> 8 files changed, 165 insertions(+), 6 deletions(-) >> create mode 100644 elf/tst-audit21.c >> create mode 100644 elf/tst-auditmod21a.c >> create mode 100644 elf/tst-auditmod21b.c >> >> diff --git a/elf/Makefile b/elf/Makefile >> index a3b4468593..7d01b71f6a 100644 >> --- a/elf/Makefile >> +++ b/elf/Makefile >> @@ -376,6 +376,7 @@ tests += \ >> tst-audit18 \ >> tst-audit19b \ >> tst-audit20 \ >> + tst-audit21 \ > > OK. Test. > >> tst-audit22 \ >> tst-audit23 \ >> tst-auditmany \ >> @@ -696,6 +697,8 @@ modules-names = \ >> tst-auditmod19a \ >> tst-auditmod19b \ >> tst-auditmod20 \ >> + tst-auditmod21a \ >> + tst-auditmod21b \ > > OK. Two audit modules. > >> tst-auditmod22 \ >> tst-auditmod23 \ >> tst-auxvalmod \ >> @@ -2136,6 +2139,11 @@ tst-audit19b-ARGS = -- $(host-test-program-cmd) >> $(objpfx)tst-audit20.out: $(objpfx)tst-auditmod20.so >> tst-audit20-ENV = LD_AUDIT=$(objpfx)tst-auditmod20.so >> >> +$(objpfx)tst-audit21: $(shared-thread-library) >> +$(objpfx)tst-audit21.out: $(objpfx)tst-auditmod21a.so >> +$(objpfx)tst-auditmod21a.so: $(objpfx)tst-auditmod21b.so > > OK. A deps B. > >> +tst-audit21-ENV = LD_AUDIT=$(objpfx)tst-auditmod21a.so > > OK. New test is audited by A, which deps B. > >> + >> $(objpfx)tst-audit22.out: $(objpfx)tst-auditmod22.so >> tst-audit22-ARGS = -- $(host-test-program-cmd) >> >> diff --git a/elf/dl-tls.c b/elf/dl-tls.c >> index 8ba70c9a9d..ccbe39660b 100644 >> --- a/elf/dl-tls.c >> +++ b/elf/dl-tls.c >> @@ -520,7 +520,7 @@ _dl_resize_dtv (dtv_t *dtv, size_t max_modid) >> >> > > This needs a comment explaining bool init_tls. > > /* Allocate initial TLS. RESULT should be a non-NULL pointer to storage > for the TLS space. The DTV may be resized, and so this function may > call malloc to allocate that space. The loader's GL(dl_load_tls_lock) > is taken when manipulating global TLS-related data in the laoder. Ack. > >> void * >> -_dl_allocate_tls_init (void *result) >> +_dl_allocate_tls_init (void *result, bool init_tls) > > OK. This is called by a reinitialization of re-used stacks. It is called > by normal _dl_allocate_tls, which is called when a thread allocates a DTV. > >> { >> if (result == NULL) >> /* The memory allocation failed. */ >> @@ -593,7 +593,14 @@ _dl_allocate_tls_init (void *result) >> some platforms use in static programs requires it. */ >> dtv[map->l_tls_modid].pointer.val = dest; >> >> - /* Copy the initialization image and clear the BSS part. */ >> + /* Copy the initialization image and clear the BSS part. For >> + audit modules or depedencies with initial-exec TLS, we can not > > s/depedencies/dependencies/g Ack. > >> + set the initial TLS image on default loader initialization >> + because it would already be set by the audit setup. However, >> + subsequent thread creation would need to follow the default >> + behaviour. */ >> + if (map->l_ns != LM_ID_BASE && !init_tls) >> + continue; > > OK. The tricky part here is that this works because GL(dl_tls_dtv_slotinfo_list) > is a *GLOBAL* list that is irrespective of linker namespace. So when we come to setup > the static TLS for the thread via rtld we will redo the already done setup for the libc > in the non-main-namespace. Yeap, that is the main issue indeed. > >> memset (__mempcpy (dest, map->l_tls_initimage, >> map->l_tls_initimage_size), '\0', >> map->l_tls_blocksize - map->l_tls_initimage_size); >> @@ -620,7 +627,7 @@ _dl_allocate_tls (void *mem) >> { >> return _dl_allocate_tls_init (mem == NULL >> ? _dl_allocate_tls_storage () >> - : allocate_dtv (mem)); >> + : allocate_dtv (mem), true); > > OK. Correct, when calling via _dl_allocate_tls we always want to initialize TLS. > >> } >> rtld_hidden_def (_dl_allocate_tls) >> >> diff --git a/elf/rtld.c b/elf/rtld.c >> index 8d233f77be..10436f7034 100644 >> --- a/elf/rtld.c >> +++ b/elf/rtld.c >> @@ -2462,7 +2462,7 @@ dl_main (const ElfW(Phdr) *phdr, >> into the main thread's TLS area, which we allocated above. >> Note: thread-local variables must only be accessed after completing >> the next step. */ >> - _dl_allocate_tls_init (tcbp); >> + _dl_allocate_tls_init (tcbp, false); > > OK. When doing this step we have already potentially initialized everything > via init_tls(naudit) earlier, but "false" is only useful if we are in the > non-base namespace. > >> >> /* And finally install it for the main thread. */ >> if (! tls_init_tp_called) >> diff --git a/elf/tst-audit21.c b/elf/tst-audit21.c >> new file mode 100644 >> index 0000000000..41446a2107 >> --- /dev/null >> +++ b/elf/tst-audit21.c >> @@ -0,0 +1,42 @@ >> +/* Check DT_AUDIT with static TLS. > > s/DT_AUDIT/LD_AUDIT/g Ack. > >> + Copyright (C) 2022 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + . */ >> + >> +#include >> +#include >> +#include >> + >> +static volatile __thread int out __attribute__ ((tls_model ("initial-exec"))); > > OK. out has size int. > >> + >> +static void * >> +tf (void *arg) >> +{ >> + TEST_COMPARE (out, 0); >> + out = isspace (' '); > > OK. Uses ctype data, which uses tls also. > >> + return NULL; >> +} >> + >> +int main (int argc, char *argv[]) >> +{ >> + TEST_COMPARE (out, 0); >> + out = isspace (' '); >> + >> + pthread_t t = xpthread_create (NULL, tf, NULL); > > OK. In the new thread look at the value. > >> + xpthread_join (t); >> + >> + return 0; >> +} >> diff --git a/elf/tst-auditmod21a.c b/elf/tst-auditmod21a.c >> new file mode 100644 >> index 0000000000..4c0f256969 >> --- /dev/null >> +++ b/elf/tst-auditmod21a.c >> @@ -0,0 +1,80 @@ >> +/* Check DT_AUDIT with static TLS. > > s/DT_AUDIT/LD_AUDIT/g Ack. > >> + Copyright (C) 2022 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + . */ >> + >> +#include >> +#include >> +#include >> + >> +#define tls_ie __attribute__ ((tls_model ("initial-exec"))) >> + >> +__thread int tls_var0 tls_ie; >> +__thread int tls_var1 tls_ie = 0x10; > > OK. Two TLS IE mode vars one with a non-zero intializer. > >> + >> +/* Defined at tst-auditmod21b.so */ >> +extern __thread int tls_var2; >> +extern __thread int tls_var3; >> + >> +static volatile int out; >> + >> +static void >> +call_libc (void) >> +{ >> + /* isspace access the initial-exec glibc TLS variables, which are > > s/access/accesses/g Ack. > >> + setup in glibc initialization. */ >> + out = isspace (' '); >> +} >> + >> +unsigned int >> +la_version (unsigned int v) >> +{ > > OK. la_version is called to initialize this module, so it's after all the > early tls initialization done by the loader. > >> + tls_var0 = 0x1; >> + if (tls_var1 != 0x10) >> + abort (); >> + tls_var1 = 0x20; >> + >> + tls_var2 = 0x2; >> + if (tls_var3 != 0x20) >> + abort (); >> + tls_var3 = 0x40; >> + >> + call_libc (); >> + >> + return LAV_CURRENT; >> +} >> + >> +unsigned int >> +la_objopen (struct link_map* map, Lmid_t lmid, uintptr_t* cookie) >> +{ >> + call_libc (); >> + *cookie = (uintptr_t) map; >> + return 0; >> +} >> + >> +void >> +la_activity (uintptr_t* cookie, unsigned int flag) >> +{ > > OK. By the time we have activity our TLS will have been overwritten. > >> + if (tls_var0 != 0x1 || tls_var1 != 0x20) >> + abort (); >> + call_libc (); >> +} >> + >> +void >> +la_preinit (uintptr_t* cookie) >> +{ >> + call_libc (); > > OK. Trigger libc tls usage after all DSOs are loaded but before main. > >> +} >> diff --git a/elf/tst-auditmod21b.c b/elf/tst-auditmod21b.c >> new file mode 100644 >> index 0000000000..69705cf75a >> --- /dev/null >> +++ b/elf/tst-auditmod21b.c >> @@ -0,0 +1,22 @@ >> +/* Check DT_AUDIT with static TLS. > > s/DT_AUDIT/LD_AUDIT/g Ack. > >> + Copyright (C) 2022 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + . */ >> + >> +#define tls_ie __attribute__ ((tls_model ("initial-exec"))) >> + >> +__thread int tls_var2 tls_ie; >> +__thread int tls_var3 tls_ie = 0x20; > > OK. > >> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c >> index 3fb085f9a1..34a33164ff 100644 >> --- a/nptl/allocatestack.c >> +++ b/nptl/allocatestack.c >> @@ -138,7 +138,7 @@ get_cached_stack (size_t *sizep, void **memp) >> memset (dtv, '\0', (dtv[-1].counter + 1) * sizeof (dtv_t)); >> >> /* Re-initialize the TLS. */ >> - _dl_allocate_tls_init (TLS_TPADJ (result)); >> + _dl_allocate_tls_init (TLS_TPADJ (result), true); > > OK. Yes, a thread needs to reinitialize this for the new stack. > >> >> return result; >> } >> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h >> index f6b2b415a6..97061bdf9f 100644 >> --- a/sysdeps/generic/ldsodefs.h >> +++ b/sysdeps/generic/ldsodefs.h >> @@ -1282,7 +1282,7 @@ 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, >> only used within rtld.c itself at startup time. */ >> extern void *_dl_allocate_tls_storage (void) attribute_hidden; >> -extern void *_dl_allocate_tls_init (void *); >> +extern void *_dl_allocate_tls_init (void *, bool); > > OK. > >> rtld_hidden_proto (_dl_allocate_tls_init) >> >> /* Deallocate memory allocated with _dl_allocate_tls. */ > >