From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.0 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.2 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by inbox.vuxu.org (OpenSMTPD) with SMTP id 7d6d5b0c for ; Wed, 26 Feb 2020 17:36:36 +0000 (UTC) Received: (qmail 32432 invoked by uid 550); 26 Feb 2020 17:36:34 -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 32414 invoked from network); 26 Feb 2020 17:36:33 -0000 Date: Wed, 26 Feb 2020 12:36:21 -0500 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20200226173621.GA11469@brightrain.aerifal.cx> References: <20200226052448.GA2769@voyager> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200226052448.GA2769@voyager> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] Add REL_COPY size change detection On Wed, Feb 26, 2020 at 06:24:48AM +0100, Markus Wichmann wrote: > Hi all, > > I was recently reading the Oracle docs about the ELF, and I came across > their chapter about the COPY relocation. They discuraged its use, since > with those relocations, a binding exists between importing and exporting > module. If the semantics of the imported object changes, then this is an > ABI mismatch. > > So I looked at the musl source code and noticed that COPY relocations > are simply processed, and an ABI mismatch is simply accepted. So, since > I am of the opinion that detectable errors should be detected, rather > than left to fester and spring a hard-to-explain bug on you, usually > five minutes before deadline, I wrote the attached patch to add > detection for at least a changed size. This won't detect all changes to > ABI regarding COPY relocation (e.g.int-->float, or in an array of > structs, a change to the struct size and to the array size cancelling > each other out), but it should find most of them. > > Also, I wondered whether COPY relocations are even still in use. But on > my system (currently some Ubuntu version) I found over 15000 of the > things. Mostly for stdout and stderr, though. In theory copy relocations should have been gone for everyone who switched to PIE, but then the gcc/binutils folks brought them back as an "optimization" (allowing pc-rel access to external symbols). :-( Anyway, I'm in agreement that we should avoid clobbering memory. The old code did that already by using sym->st_size rather than def.sym->st_size, which is why I hadn't seen this as an issue. But it can over-read (and potentially fault) if the copy reloc has a larger size than the definition, and if it's smaller the library might wrongly access past the end of the copy, reading or clobbering unrelated data. At the very least I think we ought to catch and error on the case where def.sym->st_size>sym->st_size, since we can't honor it and failure to honor it can produce silent memory corruption. I'm less sure about what to do if def.sym->st_sizest-size; this case seems safe and might be desirable not to break (I vaguely recall an intent that it be ok), but if you think there are reasons it's dangerous I'm ok with disallowing it too. I'm having a hard time now thinking of a reason it would really help to support that, anyway. > From 75e98f4e4cef2eb2b867062aebc481c3b1f66498 Mon Sep 17 00:00:00 2001 > From: Markus Wichmann > Date: Wed, 26 Feb 2020 06:09:14 +0100 > Subject: [PATCH] Add detection for changed size of a COPY relocation. > > COPY relocations create an ABI binding between importing and exporting > module. Should anything about the object in question change, that would > be an ABI change, and therefore incompatible. While the dynamic linker > is not capable of detecting all changes, it can detect most of them by > detecting a changed size between import and export. Any change is a > problem, since the source buffer will be either overread or underread. > In any case, if the semantics of the imported object changed, the ABI > contract is broken, and it is better to detect this than to silently > allow it and inexplicably crash later on. > --- > ldso/dynlink.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c > index afec985a..618c2cbd 100644 > --- a/ldso/dynlink.c > +++ b/ldso/dynlink.c > @@ -435,6 +435,15 @@ static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stri > else *reloc_addr = (size_t)base + addend; > break; > case REL_COPY: > + if (def.sym && sym->st_size != def.sym->st_size) { > + error("Error relocating %s: %s: Size mismatch in COPY" > + " relocation (exp %lu, got %lu)", > + dso->name, name > + sym->st_size + 0ul, > + def.sym->st_size + 0ul); Missing , after name, and the indent is weird (2 extra levels). I'd either align (spaces) with the opening paren or use just one indent level, and fewer lines. Semantically, st_size is size_t not ulong, so I'd probably lean towards a cast to (size_t) here with %zu. > + if (runtime) longjmp(*rtld_fail, 1); > + continue; > + } > memcpy(reloc_addr, (void *)sym_val, sym->st_size); > break; > case REL_OFFSET32: > -- Otherwise LGTM. Rich