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 2DB473858D1E for ; Wed, 30 Nov 2022 23:14:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2DB473858D1E 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: from reform (deer0x0e.wildebeest.org [172.31.17.144]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id C50DF300B36D; Thu, 1 Dec 2022 00:14:41 +0100 (CET) Received: by reform (Postfix, from userid 1000) id 818742E8055F; Thu, 1 Dec 2022 00:14:41 +0100 (CET) Date: Thu, 1 Dec 2022 00:14:41 +0100 From: Mark Wielaard To: Gavin Li Cc: elfutils-devel@sourceware.org Subject: Re: [PATCH] libdwfl: Read no more than required to parse dynamic sections Message-ID: References: <20221129062653.298772-1-gavin@matician.com> <70c0b7df5c6b38492a0655c0ff552453d9dc1f5d.camel@klomp.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="Xs5tLctFo3TFh+aj" Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-3038.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --Xs5tLctFo3TFh+aj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Gavin, On Tue, Nov 29, 2022 at 01:48:42PM -0800, Gavin Li wrote: > I think for the purposes of reading small segments (like PT_DYNAMIC > and PT_NOTE), we should ignore *buffer_available altogether. Thanks for walking me through the code. I think you are right and none of the buffer_available checks are necessary. So I removed them all. I also adjusted the commit message a bit. Could you look at this patch and let me know if this works for you? Cheers, Mark --Xs5tLctFo3TFh+aj Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-libdwfl-Read-no-more-than-required-in-dwfl_segment_r.patch" >From 59f1b49c1a1163ebde891196c1b24e3f4225915b Mon Sep 17 00:00:00 2001 From: Gavin Li Date: Wed, 30 Nov 2022 18:26:19 +0100 Subject: [PATCH] libdwfl: Read no more than required in dwfl_segment_report_module Since read_portion and the standard dwfl_elf_phdr_memory_callback functions make sure to read at least minread bytes there is no need for dwfl_segment_report_module to check and adjust the data to the actual buffer size read. Reading beyond the end of the expected data size (if the buffer read is much larger) actually causes issues when passing the data to elfXX_xlatetom() because it is possible that src->d_size is not a multiple of recsize (for ELF_T_DYN, recsize is 16 while the minimum required alignment is 8), causing elfXX_xlatetom() to return ELF_E_INVALID_DATA. Signed-off-by: Gavin Li Signed-off-by: Mark Wielaard --- libdwfl/ChangeLog | 6 ++++++ libdwfl/dwfl_segment_report_module.c | 28 +++------------------------- 2 files changed, 9 insertions(+), 25 deletions(-) diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog index 6dd84a6f..68527327 100644 --- a/libdwfl/ChangeLog +++ b/libdwfl/ChangeLog @@ -1,3 +1,9 @@ +2022-11-28 Gavin Li + Mark Wielaard + + * dwfl_segment_report_module.c (dwfl_segment_report_module): Remove + data size check after read_portion memory_callback. + 2022-10-21 Yonggang Luo * argp-std.c: Don't include unistd.h. diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 287fc002..6a7b77f0 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -441,18 +441,8 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, start + phoff, xlatefrom.d_size)) goto out; - /* ph_buffer_size will be zero if we got everything from the initial - buffer, otherwise it will be the size of the new buffer that - could be read. */ - if (ph_buffer_size != 0) - { - phnum = ph_buffer_size / phentsize; - if (phnum == 0) - goto out; - xlatefrom.d_size = ph_buffer_size; - } - xlatefrom.d_buf = ph_buffer; + xlatefrom.d_size = xlatefrom.d_size; bool class32 = ei_class == ELFCLASS32; size_t phdr_size = class32 ? sizeof (Elf32_Phdr) : sizeof (Elf64_Phdr); @@ -533,18 +523,12 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, /* We calculate from the p_offset of the note segment, because we don't yet know the bias for its p_vaddr. */ const GElf_Addr note_vaddr = start + offset; - void *data; - size_t data_size; + void *data = NULL; + size_t data_size = 0; if (read_portion (&read_state, &data, &data_size, start, segment, note_vaddr, filesz)) continue; /* Next header */ - /* data_size will be zero if we got everything from the initial - buffer, otherwise it will be the size of the new buffer that - could be read. */ - if (data_size != 0) - filesz = data_size; - if (filesz > SIZE_MAX / sizeof (Elf32_Nhdr)) continue; @@ -821,12 +805,6 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name, && ! read_portion (&read_state, &dyn_data, &dyn_data_size, start, segment, dyn_vaddr, dyn_filesz)) { - /* dyn_data_size will be zero if we got everything from the initial - buffer, otherwise it will be the size of the new buffer that - could be read. */ - if (dyn_data_size != 0) - dyn_filesz = dyn_data_size; - if ((dyn_filesz / dyn_entsize) == 0 || dyn_filesz > (SIZE_MAX / dyn_entsize)) goto out; -- 2.30.2 --Xs5tLctFo3TFh+aj--