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 6B61E3858D33 for ; Tue, 7 Feb 2023 14:11:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6B61E3858D33 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 5068D92009C; Tue, 7 Feb 2023 15:11:23 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by angie.orcam.me.uk (Postfix) with ESMTP id 49E7292009B; Tue, 7 Feb 2023 14:11:23 +0000 (GMT) Date: Tue, 7 Feb 2023 14:11:23 +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 Mon, 6 Feb 2023, Alan Modra wrote: > This is another fix for the testcase mentioned in > https://sourceware.org/pipermail/binutils/2023-February/125915.html > either of which will stop the addr2line segfault. This one also fixes > a potential problem when linking corrupted debug info. Hmm, from the other message I gather DWARF info is not going to be processed twice anymore, so why is this change to the MIPS backend also required? 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. 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... > + while (tdata->mips_hi16_list != NULL) > + { > + bfd_reloc_status_type ret; > + struct mips_hi16 *hi; > + > + hi = tdata->mips_hi16_list; > + > + /* R_MIPS*_GOT16 relocations are something of a special case. > + We want to install the addend in the same way as for a > + R_MIPS*_HI16 relocation (with a rightshift of 16). > + However, since GOT16 relocations can also be used with > + global symbols, their howto has a rightshift of 0. */ > + if (hi->rel.howto->type == R_MIPS_GOT16) > + hi->rel.howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, R_MIPS_HI16, false); > + else if (hi->rel.howto->type == R_MIPS16_GOT16) > + hi->rel.howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, R_MIPS16_HI16, false); > + else if (hi->rel.howto->type == R_MICROMIPS_GOT16) > + hi->rel.howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, R_MICROMIPS_HI16, > + false); > + > + /* VALLO is a signed 16-bit number. Bias it by 0x8000 so that > + any carry or borrow will induce a change of +1 or -1 in the > + high part. */ > + hi->rel.addend += (vallo + 0x8000) & 0xffff; ... here. > + /* 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. */ 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. Maciej