From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from angie.orcam.me.uk (angie.orcam.me.uk [78.133.224.34]) by sourceware.org (Postfix) with ESMTP id 52E963858D39 for ; Wed, 8 Feb 2023 23:28:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 52E963858D39 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=orcam.me.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=orcam.me.uk Received: by angie.orcam.me.uk (Postfix, from userid 500) id E7CFE92009C; Thu, 9 Feb 2023 00:28:08 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by angie.orcam.me.uk (Postfix) with ESMTP id E07A292009B; Wed, 8 Feb 2023 23:28:08 +0000 (GMT) Date: Wed, 8 Feb 2023 23:28:08 +0000 (GMT) From: "Maciej W. Rozycki" To: Alan Modra cc: binutils@sourceware.org, Chenghua Xu Subject: Re: Protect mips_hi16_list from fuzzed debug info In-Reply-To: Message-ID: References: User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-1163.1 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,KAM_INFOUSMEBIZ,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, 8 Feb 2023, Alan Modra wrote: > > Also would it be possible to have a MIPS test case for your change? > > Orchestrating HI16/LO16 relocations in a debug section should be pretty > > straightforward with the use of the `.reloc' pseudo-op. This might help > > me understand what is really going on here. > > I could do that, but my time is limited for mips problems. Completely understood (and mine BTW too). > I'll > understand if you say the patch is not worth committing just to cover > a potential fuzzed object file segfault. No, not at all, I agree we do need to maintain at least basic quality, and preventing crashes from happening even for garbage input is about the minimum required. > > Also one concern about code proposed itself, see below. > > > > > @@ -2596,38 +2600,41 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol, > > [...] > > > + if (!tdata->freeze_mips_hi16_list) > > > > This conditional ought to wrap all the preceding code in the function as > > well (including the declaration block), because it's sole purpose is to > > retrieve `vallo', which is only used within the `while' loop now placed > > under the conditional... > > OK, done. I'm presuming I don't need to repost the patch. Sure, no need to repost. > > > + /* Debug info should not contain hi16 or lo16 relocs. If it does > > > + then someone is playing fuzzing games. Altering the hi16 list > > > + during linking when printing an error message is bad. */ > > s/an error/a warning/ Thanks, that changes things a bit. > > And I really cannot extract the meaning of the second sentence here. I > > mean I know what it literally means, but that does not really tell me > > anything. Why would altering the list be a problem given that we're > > bailing out anyway? I'm confused. > > A number of the error/warning handlers in ldmain.c use %C. This can > cause debug info to be parsed for the first time in order to print > file/function/line. Hmm, I find it an interesting general phenomenon. What it means the order sections are processed in can change depending on whether a warning has been issued in the course or not. Is it not a problem in the first place? Shouldn't we give priority to debugs sections and parse them first then before moving on to the other sections? > If one of those warnings is triggered after some > hi16 relocs have been processed but before the matching lo16 reloc is > handled, *and* the debug info is corrupted with a lo16 reloc, then the > mips_hi16_list will be flushed with the result that printing a warning > changes linker output. OK, but wouldn't these relocs be resolved in the same problematic way anyway when the turn came to processing debug sections? > It is also possible that corrupted debug info > adds to the hi16 list, with the result that when the linker handles a > later lo16 reloc in a text section, ld will segfault accessing > mips_hi16.data after the debug buffers have be freed. This smells a HI16/LO16 pair processing bug to me by itself. Such pairs must come from the same relocation section, so any HI16/LO16 relocations in a relocation section associated with a debug section are not supposed to influence any such relocations referring to the text section. I think I need to look into it (though see above as to my availability). > Is this likely > to happen in the real world? No, of course not, but fuzzers keep > finding this sort of thing, and the occasional real problem found by > the fuzzers is enough that I haven't yet decided to ignore all fuzzing > reports. Well, we must not crash, period! I hope there's no hurry with this change, so please let me chew it over yet for a couple days. Long-term I think the MIPS target would benefit from proper day-to-day maintenance (and I can't just clone myself no matter how much I might desire). Maciej