From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.1 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 91CBB21327 for ; Thu, 29 Feb 2024 19:16:14 +0100 (CET) Received: (qmail 5294 invoked by uid 550); 29 Feb 2024 18:12:31 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 5258 invoked from network); 29 Feb 2024 18:12:30 -0000 Date: Thu, 29 Feb 2024 13:16:18 -0500 From: Rich Felker To: Max Filippov Cc: musl@lists.openwall.com Message-ID: <20240229181617.GW4163@brightrain.aerifal.cx> References: <20240227232430.GM4163@brightrain.aerifal.cx> <20240228001303.GN4163@brightrain.aerifal.cx> <20240228183032.GO4163@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] Initial xtensa/fdpic port review On Thu, Feb 29, 2024 at 08:25:12AM -0800, Max Filippov wrote: > On Wed, Feb 28, 2024 at 10:30 AM Rich Felker wrote: > > On Wed, Feb 28, 2024 at 09:20:33AM -0800, Max Filippov wrote: > > > > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c > > > > > index ceca3c98..25563af3 100644 > > > > > --- a/ldso/dynlink.c > > > > > +++ b/ldso/dynlink.c > > > > > @@ -1420,6 +1420,7 @@ static void reloc_all(struct dso *p) > > > > > if (!DL_FDPIC) > > > > > do_relr_relocs(p, laddr(p, dyn[DT_RELR]), dyn[DT_RELRSZ]); > > > > > > > > > > +#if 0 > > > > > if (head != &ldso && p->relro_start != p->relro_end) { > > > > > long ret = __syscall(SYS_mprotect, laddr(p, p->relro_start), > > > > > p->relro_end-p->relro_start, PROT_READ); > > > > > @@ -1429,6 +1430,7 @@ static void reloc_all(struct dso *p) > > > > > if (runtime) longjmp(*rtld_fail, 1); > > > > > } > > > > > } > > > > > +#endif > > > > > > > > Was this breaking something? Relro linking probably should have been > > > > disabled on fdpic, and we ignore ENOSYS anyway for nommu. > > > > > > Yeah, I do some of the testing in linux-user QEMU, it has MMU > > > and mprotect calls actually succeed. Failures were coming from the > > > relocation code, but my impression was that relro should be applied > > > after the relocation completion and it should just work, hence the > > > WIP pile. > > > > Yes, relro is only supposed to be applied after all relocations were > > done. > > So I've found two issues with this. First is that loadmap entries generated > by the QEMU (and by the linux kernel AFAICS) are not page-aligned, > but the relro segment addresses fetched by the loader are. Since the > laddr() doesn't check that a translation for the address it was given > was actually found the results are not always correct, mprotect fails > and it all terminates early. > > With a workaround for that part in place I see that relro protection is > applied to the executable image before its __fdpic_fixup had a chance > to run, there are rofixups for the .init_array and .fini_array, but they both > are a part of the relro segment. OK, in that case, we probably should disable relro with fdpic until it's fixed to work properly. I think relro processing for the main executable should probably be moved to run as the first step of __libc_start_init, so that it happens after __fdpic_fixup has run. We also need to either make relro_start and relro_end maintain the exact addresses and only get expanded to full-page just before calling mprotect, or use laddr_pg to get the containing page. Unrelated to fdpic, there may also be an issue that relro processing has a broken dependency on runtime-variable pagesize. In practice it's probaby okay since the binary must have been linked for maxpagesize >= PAGE_SIZE, but we should probably double-check if the current alignment logic is semantically correct. Rich