From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 3D58D396E829 for ; Wed, 8 Jun 2022 19:48:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3D58D396E829 Received: by mail-pg1-x52f.google.com with SMTP id 184so3924477pga.12 for ; Wed, 08 Jun 2022 12:48:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=MqRbSB7acDru/RV63nE1n8RrGRIGdkXCZcFUjIQVFB4=; b=n0rm1WftsuqYXRsrYlUKal7Nj4B9Xb4q52sY4wcYzRx6LJRrXrF/o0ozKuJ3AqBMwo LAJEuCbLblN2yDyyHZ+GlRmIl4ncgLYkzu19rmtddprh7VFDvzms/GP32jx/qQetKlNf EMvHM3p/LnH8eijZj9Zsg0AAjAIKeqqZtFAOLUE8O16xyRA/x28mliFhY5t8y2Fx/IIi z6uFreb+mbihqFb3cy5Xv8JVea7FrAYzJg5arX+toQMXPy9h3sLvkte0p6F9Fzo+WEOd kNyzeWRIexIq22a7cYj+G16/Lb3VV6fQL24z2imkOQZ1+FTM6K8MRlQvc85/+0UuLTsU FlFA== X-Gm-Message-State: AOAM533/d4DaCYEqd0KkOfvdT58OF86NhO5owrZYmWE+07+xLIDhr2DV wY50K21EZsW4puLFiKNusk9PjyX62B2K4w== X-Google-Smtp-Source: ABdhPJxAIEz+W/r5ZimVPedZI/OzEENZdnkQTU7pZGK/BtiHD95X2HanZxReKqONgfAUOBdRtOZmOA== X-Received: by 2002:a05:6a00:16cf:b0:51b:5f55:9bc with SMTP id l15-20020a056a0016cf00b0051b5f5509bcmr35906934pfc.7.1654717692039; Wed, 08 Jun 2022 12:48:12 -0700 (PDT) Received: from google.com ([2620:15c:2ce:200:6028:be76:46d0:2a86]) by smtp.gmail.com with ESMTPSA id l19-20020a170902d35300b0015e8d4eb2easm15008478plk.308.2022.06.08.12.48.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 08 Jun 2022 12:48:11 -0700 (PDT) Date: Wed, 8 Jun 2022 12:48:08 -0700 From: Fangrui Song To: "H.J. Lu" Cc: GNU C Library Subject: Re: [PATCH] elf: Refine direct extern access diagnostics to protected symbol Message-ID: <20220608194808.kucpfiir4prwuym5@google.com> References: <20220607235308.1190097-1-maskray@google.com> <20220608035319.zby4thtncystuevt@google.com> <20220608185907.4mfcb6t6bjlssj22@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-27.4 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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, 08 Jun 2022 19:48:15 -0000 On 2022-06-08, H.J. Lu wrote: >On Wed, Jun 8, 2022 at 11:59 AM Fangrui Song wrote: >> >> On 2022-06-08, H.J. Lu wrote: >> >On Tue, Jun 7, 2022 at 8:53 PM Fangrui Song wrote: >> >> >> >> On 2022-06-07, H.J. Lu wrote: >> >> >On Tue, Jun 7, 2022 at 4:53 PM Fangrui Song via Libc-alpha >> >> > wrote: >> >> >> >> >> >> Refine commit 349b0441dab375099b1d7f6909c1742286a67da9: >> >> >> >> >> >> 1. Copy relocations for extern protected data do not work properly, >> >> >> regardless whether GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS is used. >> >> >> It makes sense to produce a warning unconditionally. When the defining >> >> >> shared object has GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS, report >> >> >> an error to satisfy the "no copy relocations" enforcement intended by >> >> >> this GNU property. >> >> >> >> >> >> 2. Non-zero value of an undefined function symbol may break pointer >> >> >> equality, but may be benign in many cases (many programs don't take the >> >> >> address in the shared object then compare it with the address in the >> >> >> executable). Report a warning instead. While here, reword the >> >> >> diagnostic to be clearer. >> >> >> >> >> >> 3. Remove the unneeded condition !(undef_map->l_1_needed & >> >> >> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS). If the executable has >> >> >> GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS (can only occur in error >> >> >> cases), the diagnostic should be emitted as well. >> >> >> --- >> >> >> sysdeps/generic/dl-protected.h | 46 ++++++++++++++++++---------------- >> >> >> 1 file changed, 25 insertions(+), 21 deletions(-) >> >> >> >> >> >> diff --git a/sysdeps/generic/dl-protected.h b/sysdeps/generic/dl-protected.h >> >> >> index 88cb8ec917..ed40d9fea9 100644 >> >> >> --- a/sysdeps/generic/dl-protected.h >> >> >> +++ b/sysdeps/generic/dl-protected.h >> >> >> @@ -26,29 +26,33 @@ _dl_check_protected_symbol (const char *undef_name, >> >> >> const struct link_map *map, >> >> >> int type_class) >> >> >> { >> >> >> - if (undef_map != NULL >> >> >> - && undef_map->l_type == lt_executable >> >> >> - && !(undef_map->l_1_needed >> >> >> - & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS) >> >> >> - && (map->l_1_needed >> >> >> - & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS)) >> >> >> + if (undef_map == NULL || undef_map->l_type != lt_executable) >> >> >> + return; >> >> >> + >> >> >> + if (type_class & ELF_RTYPE_CLASS_COPY) >> >> >> { >> >> >> - if ((type_class & ELF_RTYPE_CLASS_COPY)) >> >> >> - /* Disallow copy relocations in executable against protected >> >> >> - data symbols in a shared object which needs indirect external >> >> >> - access. */ >> >> >> - _dl_signal_error (0, map->l_name, undef_name, >> >> >> - N_("copy relocation against non-copyable protected symbol")); >> >> >> - else if (ref->st_value != 0 >> >> >> - && ref->st_shndx == SHN_UNDEF >> >> >> - && (type_class & ELF_RTYPE_CLASS_PLT)) >> >> >> - /* Disallow non-zero symbol values of undefined symbols in >> >> >> - executable, which are used as the function pointer, against >> >> >> - protected function symbols in a shared object with indirect >> >> >> - external access. */ >> >> >> - _dl_signal_error (0, map->l_name, undef_name, >> >> >> - N_("non-canonical reference to canonical protected function")); >> >> >> + /* Disallow copy relocations in executable against protected >> >> >> + data symbols in a shared object which needs indirect external >> >> >> + access. */ >> >> >> + _dl_error_printf ("warning: copy relocation against non-copyable " >> >> >> + "protected symbol `%s' in `%s'\n", >> >> >> + undef_name, map->l_name); >> >> >> + >> >> >> + if (map->l_1_needed & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS) >> >> >> + _dl_signal_error ( >> >> >> + 0, map->l_name, undef_name, >> >> >> + N_ ("error due to GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS")); >> >> >> } >> >> >> + else if ((type_class & ELF_RTYPE_CLASS_PLT) && ref->st_value != 0 >> >> >> + && ref->st_shndx == SHN_UNDEF) >> >> >> + /* Disallow non-zero symbol values of undefined symbols in >> >> >> + executable, which are used as the function pointer, against >> >> >> + protected function symbols in a shared object with indirect >> >> >> + external access. */ >> >> >> + _dl_error_printf ( >> >> >> + "warning: direct reference to " >> >> >> + "protected function `%s' in `%s' may break pointer equality\n", >> >> >> + undef_name, map->l_name); >> >> > >> >> >Should there be an error for GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS? >> >> >> >> I lean toward a warning, as bullet point 2 in my commit message >> >> explains. >> > >> >It is due to R_386_PC32. Can we make the error optional and enable it >> >for x86-64? >> >> Do you mean that the branch should call call _dl_signal_error in the >> presence of GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS? > >Yes. Sent v2 https://sourceware.org/pipermail/libc-alpha/2022-June/139587.html >> else if ((type_class & ELF_RTYPE_CLASS_PLT) && ref->st_value != 0 >> && ref->st_shndx == SHN_UNDEF) >> >> >> In addition, this check requires that the executable with a non-zero >> >> st_value has at least one JUMP_SLOT relocation. >> >> >> >> In the following setup the executable does not have JUMP_SLOT, so >> >> there is no diagnostic, with or without the patch. >> > >> >We can pass symbol definition and check STT_FUNC. >> >> My point is that the check only kicks in when there is a dynamic >> relocation using ELF_RTYPE_CLASS_PLT (typically JUMP_SLOT). If the >> executable just takes the address but doesn't call the function, the >> branch will not be executed at all. > >Yes, linker has resolved the relocation to the PLT entry. There may be >no dynamic relocation. Replacing R_386_PC32 with R_386_PLT32 >won't work correctly since R_386_PLT32 assumes EBX setup and >R_386_PC32 doesn't. Linker needs to handle it properly for protected symbols. I see that you categorize the two relocations (which can be used as branches) this way: * R_386_PC32: no-pic PLT or address-taking. Needs to check SHF_EXECINSTR and disassembly the insn * R_386_PLT32: pic PLT and therefore think that `jmp foo` cannot switch to R_386_PC32. However, I categorize them this way: * R_386_PC32: possibly address-taking * R_386_PLT32: branch (either no-pic or pic PLT). -no-pie uses no-pic PLT while -pie/-shared uses pic PLT My way follows the intention of R_X86_64_PC32/R_X86_64_PLT32. >> That said, I am flexible and can add the wrong if you feel strong about >> it. To be clear, do you indicate that the error should require >> !defined(__i386__) ? > >You can add a macro to disable the error by default and x86-64 can >opt it in. My PATCH v2 doesn't do anything with __i386__. The R_386_PC32 concern is a corner case (a shared object has upgraded from STV_DEFAULT to STV_PROTECTED, lld is used, there is a -fno-pic executable using neither -fno-direct-access-external-data -mno-direct-extern-access). Seems good to make it separate even if we decide to do something. >> >> // a.c -fno-pic -no-pie >> >> #include >> >> int foo(void); >> >> int main(void) { printf("%p\n", foo); >> >> >> >> // b.c -fpic -shared >> >> int foo(void) { return 3; } >> >> asm(".protected foo"); >> >> >> >> >> } >> >> >> >> >> >> #endif /* _DL_PROTECTED_H */ >> >> >> -- >> >> >> 2.36.1.255.ge46751e96f-goog >> >> >> >> >> > >> >> > >> >> >-- >> >> >H.J. >> > >> > >> > >> >-- >> >H.J. > > > >-- >H.J.