From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id 19DAD3858D33 for ; Thu, 16 Feb 2023 23:45:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 19DAD3858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: by gnu.wildebeest.org (Postfix, from userid 1000) id 597AE3000648; Fri, 17 Feb 2023 00:45:50 +0100 (CET) Date: Fri, 17 Feb 2023 00:45:50 +0100 From: Mark Wielaard To: Aleksei Vetrov Cc: elfutils-devel@sourceware.org, kernel-team@android.com, maennich@google.com Subject: Re: [PATCH v2 1/1] libdw: check that DWARF strings are null-terminated Message-ID: <20230216234550.GI6028@gnu.wildebeest.org> References: <20230214200042.3675502-1-vvvvvv@google.com> <77e629c6d4992af3701c2e4bd79dae2df99a1737.1676406334.git.vvvvvv@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <77e629c6d4992af3701c2e4bd79dae2df99a1737.1676406334.git.vvvvvv@google.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-3031.3 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,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: Hi Aleksei, On Tue, Feb 14, 2023 at 08:30:02PM +0000, Aleksei Vetrov via Elfutils-devel wrote: > It is expected from libdw to return strings that are null-terminated to > avoid overflowing ELF data. > > * Add calculation of a safe prefix inside string sections, where any > string will be null-terminated. > > * Check if offset overflows the safe prefix in dwarf_formstring. This is a very nice sanity/hardening fix. > + /* If the section contains string data, we want to know a size of a prefix > + where any string will be null-terminated. */ > + enum string_section_index string_section_idx = scn_to_string_section_idx[cnt]; > + if (string_section_idx < STR_SCN_IDX_last) > + { > + size_t size = data->d_size; > + /* Reduce the size by the number of non-zero bytes at the end of the > + section. */ > + while (size > 0 && *((const char *) data->d_buf + size - 1) != '\0') > + --size; > + result->string_section_size[string_section_idx] = size; > + } Checking from the end is smart. In the normal case the debug string section will end with a zero terminator (or zero padding), so this should be really quick. > @@ -171,7 +174,7 @@ dwarf_formstring (Dwarf_Attribute *attrp) > else > off = read_8ubyte_unaligned (dbg, datap); > > - if (off > dbg->sectiondata[IDX_debug_str]->d_size) > + if (off >= data_size) > goto invalid_offset; > } O, the original check was actually one off. Looks good. Pushed as is. Thanks, Mark