From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x835.google.com (mail-qt1-x835.google.com [IPv6:2607:f8b0:4864:20::835]) by sourceware.org (Postfix) with ESMTPS id 1C35E3898526 for ; Wed, 7 Apr 2021 14:28:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1C35E3898526 Received: by mail-qt1-x835.google.com with SMTP id i19so13841808qtv.7 for ; Wed, 07 Apr 2021 07:28:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=D/74QzGza57ps0keGjaEjs6gHaDw0B9Ju+DhorEyVN8=; b=EwzmkGs6+8ka/BTFgo/49z15t3BWbg/q7Y4eqFLTGG4QcpgUab7dy1F8YF4ADUikcg U9fdMaPaUKTkBqq/pDIIJguhFffxKYd2D96IfitNyAj9Ssf9CpVYpD25fQG474iUIYrp 02WZ9ZRSPoNGHWFQlRe9W/o17sD1e5JsJnGtFHnoFuSGCrDDlvS4l9iqRrwibtgbVuak I5i6RtI7F6RfIBdzHdNNviExbNn78EYMIN8lQRLd6xHCjWhibi4YHlDN2dJsJLBr0Np/ 8CuLlashrmpTnp3KEal8zxYtpXOVxKkPPUkqhQ5Qf1oRryohsgm4bduh+aTVx2SkDq5u BL0A== X-Gm-Message-State: AOAM53056ENHG1NetCuDvYJ7sfS4FwieK7vDvrv8zGbYeZjoYIuwW4jG hvTPaLK4vrYoXxYdpZLA8H/UcMcgU+lGC/tO X-Google-Smtp-Source: ABdhPJzoO3J5A1Kx2fCUIdC0n3KMQTGz43UatbRk6YoGWXnSS7e1PZ0Okc3ygctAwmwcMHcLbOyqOA== X-Received: by 2002:ac8:67d3:: with SMTP id r19mr2934879qtp.23.1617805690474; Wed, 07 Apr 2021 07:28:10 -0700 (PDT) Received: from [192.168.1.132] ([177.194.41.149]) by smtp.gmail.com with ESMTPSA id y9sm17975646qkm.19.2021.04.07.07.28.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 07 Apr 2021 07:28:10 -0700 (PDT) Subject: Re: [PATCH 07/15] elf: Refactor _dl_update_slotinfo to avoid use after free To: Szabolcs Nagy Cc: libc-alpha@sourceware.org References: <3ecdb956cbf6d1b46e36311ffe7f491ce186cdbc.1613390045.git.szabolcs.nagy@arm.com> <2f925732-4c85-4dfe-036e-ed2dde651202@linaro.org> <20210407080109.GP23289@arm.com> From: Adhemerval Zanella Message-ID: <1ccd8aa7-ff8e-96e7-7bf8-d229572d1a27@linaro.org> Date: Wed, 7 Apr 2021 11:28:08 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210407080109.GP23289@arm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Wed, 07 Apr 2021 14:28:12 -0000 On 07/04/2021 05:01, Szabolcs Nagy wrote: > The 04/06/2021 16:40, Adhemerval Zanella wrote: >> On 15/02/2021 09:00, Szabolcs Nagy via Libc-alpha wrote: >>> map is not valid to access here because it can be freed by a >>> concurrent dlclose, so don't check the modid. >> >> Won't it be protected by the recursive GL(dl_load_lock) in such case? >> I think the concurrency issue is between dlopen and _dl_deallocate_tls >> called by pthread stack handling (nptl/allocatestack.c). Am I missing >> something here? > > > _dl_update_slotinfo is called both with and without > the dlopen lock held: during dynamic tls access > the lock is not held (see the __tls_get_addr path) Right, revising the patch I mapped the calls (not sure if it is fully complete): | _dl_open | __rtld_lock_lock_recursive (GL(dl_load_lock)); | dl_open_worker | update_tls_slotinfo | _dl_update_slotinfo | __rtld_lock_unlock_recursive (GL(dl_load_lock)); | __tls_get_addr | update_get_addr | _dl_update_slotinfo | rtld | _dl_resolve_conflicts | elf_machine_rela | TRY_STATIC_TLS | _dl_try_allocate_static_tls | _dl_update_slotinfo | | elf_machine_rela | CHECK_STATIC_TLS | _dl_allocate_static_tls | _dl_try_allocate_static_tls | _dl_update_slotinfo The rtld part should not matter, since it is done before thread is supported. > > we cannot add a lock there because that would cause > new deadlocks, dealing with this is the tricky part > of the patchset. I understand this patch from previous discussion about it. The part is confusing me is "because it can be freed by a concurrent dlclose". My understanding is '_dl_deallocate_tls' might be called in thread exit / deallocation without the GL(dl_load_lock) (which is a potential issue); what I can't see is how concurrent dlclose might trigger this issue (since it should be synchronized with dlopen through the lock). > >>> >>> The map == 0 and map != 0 code paths can be shared (avoiding >>> the dtv resize in case of map == 0 is just an optimization: >>> larger dtv than necessary would be fine too). >>> --- >>> elf/dl-tls.c | 21 +++++---------------- >>> 1 file changed, 5 insertions(+), 16 deletions(-) >>> >>> diff --git a/elf/dl-tls.c b/elf/dl-tls.c >>> index 24d00c14ef..f8b32b3ecb 100644 >>> --- a/elf/dl-tls.c >>> +++ b/elf/dl-tls.c >>> @@ -743,6 +743,8 @@ _dl_update_slotinfo (unsigned long int req_modid) >>> { >>> for (size_t cnt = total == 0 ? 1 : 0; cnt < listp->len; ++cnt) >>> { >>> + size_t modid = total + cnt; >>> + >>> size_t gen = listp->slotinfo[cnt].gen; >>> >>> if (gen > new_gen) >>> @@ -758,25 +760,12 @@ _dl_update_slotinfo (unsigned long int req_modid) >>> >>> /* If there is no map this means the entry is empty. */ >>> struct link_map *map = listp->slotinfo[cnt].map; >>> - if (map == NULL) >>> - { >>> - if (dtv[-1].counter >= total + cnt) >>> - { >>> - /* If this modid was used at some point the memory >>> - might still be allocated. */ >>> - free (dtv[total + cnt].pointer.to_free); >>> - dtv[total + cnt].pointer.val = TLS_DTV_UNALLOCATED; >>> - dtv[total + cnt].pointer.to_free = NULL; >>> - } >>> - >>> - continue; >>> - } >>> - >>> /* Check whether the current dtv array is large enough. */ >>> - size_t modid = map->l_tls_modid; >>> - assert (total + cnt == modid); >>> if (dtv[-1].counter < modid) >>> { >>> + if (map == NULL) >>> + continue; >>> + >>> /* Resize the dtv. */ >>> dtv = _dl_resize_dtv (dtv); >>> >>>