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 2ADD23858D1E for ; Tue, 29 Nov 2022 15:25:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2ADD23858D1E 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 tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 03108300B351; Tue, 29 Nov 2022 16:25:36 +0100 (CET) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 5FE484000C75; Tue, 29 Nov 2022 16:25:36 +0100 (CET) Message-ID: <70c0b7df5c6b38492a0655c0ff552453d9dc1f5d.camel@klomp.org> Subject: Re: [PATCH] libdwfl: Read no more than required to parse dynamic sections From: Mark Wielaard To: gavin@matician.com, elfutils-devel@sourceware.org Date: Tue, 29 Nov 2022 16:25:36 +0100 In-Reply-To: <20221129062653.298772-1-gavin@matician.com> References: <20221129062653.298772-1-gavin@matician.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-3032.8 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 Gavin, On Mon, 2022-11-28 at 22:26 -0800, gavin@matician.com wrote: > Since size checking has been moved to > dwfl_elf_phdr_memory_callback(), > there is no longer a need for dwfl_segment_report_module() to enforce > the same. Reading beyond the end of the dynamic section 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. I don't fully follow this logic. The code as written doesn't seem to guarantee that dwfl_segment_report_module will always be called with dwfl_elf_phdr_memory_callback as memory_callback. Although it probably will be in practice. So you are removing this check: > && ! 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 initia= l > - buffer, otherwise it will be the size of the new buffer that > - could be read. */ > - if (dyn_data_size !=3D 0) > - dyn_filesz =3D dyn_data_size; > - Reading read_portion it shows dyn_data_size being set to zero if read_state->buffer_available has everything (dyn_filesz) requested. Otherwise memory_callback (we assume dwfl_elf_phdr_memory_callback) is called with *data_size =3D dyn_filesz. Which will then be set to the actual buffer size being read. So dyn_data_size might be bigger than the dynfilesz we are requesting? Or smaller I assume. If you are protecting against it becoming bigger, should the check be changed to: if (dyn_data_size !=3D 0 && dyn_data_size < dyn_filesz) dyn_filesz =3D dyn_data_size; ? Thanks, Mark