Message ID | 20221129062653.298772-1-gavin@matician.com |
---|---|
State | Committed |
Headers |
Return-Path: <elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D9F2438582A3 for <patchwork@sourceware.org>; Tue, 29 Nov 2022 06:27:05 +0000 (GMT) X-Original-To: elfutils-devel@sourceware.org Delivered-To: elfutils-devel@sourceware.org Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id AA2363858D33 for <elfutils-devel@sourceware.org>; Tue, 29 Nov 2022 06:26:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AA2363858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=matician.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=matician.com Received: by mail-pg1-x52f.google.com with SMTP id f9so12098448pgf.7 for <elfutils-devel@sourceware.org>; Mon, 28 Nov 2022 22:26:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=matician-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=ALahRdDjFofqX8MFp66/J0D2hKccwEqBHj3ZCnbd2Is=; b=vdUB6ufzsAi+Kd2MmDWJFhL+R7XPpLwLDjCzPYCPQ/m+mKhPhWxE2wOM6fz57vGsBf jeR1UZ5rctxYSvymU9BAJfStQ7Na9PDfbH0mVuoXPNc5kL7aRm3Tg28vQyp4rgguEAt7 LOMSjoBIJJjxmDmBoRMeG/QoypxvxrAvqwuOELSSC/1cVPDr42XWJDuh76hRjMZf1xfk zaoFr7YgTIZNkyAU4pimpfyqzAklA1B03FifIKRJIdON4o7/iXW9FTvrDw4NwaznpgIt fPJJGwF5QWLEQmqeWjgacDB2lGHYm/jnD1proJR5tbpcG4aDUBwpSO90wO180qKKr6Zv HAVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ALahRdDjFofqX8MFp66/J0D2hKccwEqBHj3ZCnbd2Is=; b=A2fA+PHGfPd4FLkYjIeGVE4kOdqUnERR5JbUaLkuq4HZEVRGGGyyusbEfqZQVTINOx 1GbAU2s28zaRRCxMHbWfC/MA8ntcoN3Dh0uQGC/RCs+6cUvbJneglpxzIughDWaF0r0T FYw9i71j+aj1CHBVg4e/dpIJJsKpdOHOTArYkzTdJ8J3tDPt8FMpyHFXqln8raOPJELr dfP+F+N4PuqAutsuzeFpfTOSdiCyKc6WIWgOfLuJ+N/delKQ8S9HYdBkEWM0Rpdsspw4 s1pRhQorM+MNs3Ezv/40636DiZIz8sa76dQ+8fix/gb1h4oJsOoD58BxWYhzDQJ4Xv2C NxvA== X-Gm-Message-State: ANoB5pla8MSbwhlbQJxlD7a+6lIBS/ZeGx1JB33yEVRUS2eNe3bmSi7F Uk3hdp8a9EIzfuEuthF+fy5P82mj22eMmQ/JL9NwsPSvuOPkgm0+fXvbsV7uMdPsyCGO8yikfe8 3LqF8sxpsiHytD2xoodkMkzLliGUEvtBOtwtR6xp5Y58HQALY/J4kq1p0ahavpny2haHNWMSsL/ ylbg== X-Google-Smtp-Source: AA0mqf5MvZkLKRWgHlZJV51673aIfQLu69BmWhje0O4chxVFlrBd8pbtc3xLGiorr0b1ClelnAUKqg== X-Received: by 2002:a62:828c:0:b0:574:ae6f:61b7 with SMTP id w134-20020a62828c000000b00574ae6f61b7mr21053334pfd.26.1669703217378; Mon, 28 Nov 2022 22:26:57 -0800 (PST) Received: from localhost.localdomain ([2600:1700:42f0:8090::48]) by smtp.gmail.com with ESMTPSA id g8-20020a1709026b4800b001891ea4d133sm9898272plt.12.2022.11.28.22.26.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Nov 2022 22:26:57 -0800 (PST) From: gavin@matician.com To: elfutils-devel@sourceware.org Cc: Mark Wielaard <mark@klomp.org>, Gavin Li <gavin@matician.com> Subject: [PATCH] libdwfl: Read no more than required to parse dynamic sections Date: Mon, 28 Nov 2022 22:26:53 -0800 Message-Id: <20221129062653.298772-1-gavin@matician.com> X-Mailer: git-send-email 2.38.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, 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 X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list <elfutils-devel.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/elfutils-devel>, <mailto:elfutils-devel-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/elfutils-devel/> List-Help: <mailto:elfutils-devel-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/elfutils-devel>, <mailto:elfutils-devel-request@sourceware.org?subject=subscribe> Errors-To: elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org Sender: "Elfutils-devel" <elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
libdwfl: Read no more than required to parse dynamic sections
|
|
Commit Message
Gavin Li
Nov. 29, 2022, 6:26 a.m. UTC
From: Gavin Li <gavin@matician.com> 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. Signed-off-by: Gavin Li <gavin@matician.com> --- libdwfl/dwfl_segment_report_module.c | 6 ------ 1 file changed, 6 deletions(-)
Comments
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 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; > - 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 = 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 != 0 && dyn_data_size < dyn_filesz) dyn_filesz = dyn_data_size; ? Thanks, Mark
Hi Mark, Thanks for looking over this patch. Responses are inline. > 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. All file/line references relate to commit 98bdf533c4990728f0db653ab4e98a503d7654ce. dwfl_segment_report_module is an internal function that is currently only called from dwfl_core_file_report. Because of this, I think it would be fine to enforce that memory_callback implementations must enforce minread or return an error. dwfl_elf_phdr_memory_callback does return an error at core-file.c:336 if the amount that is able to be read is less than minread. dwfl_segment_report_module.c:340 does not buffer_available either, since it assumes that memory_callback will return an error if it is unable to read sizeof(Elf64_Ehdr) bytes. The main issue I am currently seeing is that dwfl_elf_phdr_memory_callback can return a *buffer_available that is sometimes much larger than minread, especially if the ELF file is mmaped (elf->map_address != NULL). See core-file.c:324-325. For example, on my core file, opened with elf_begin(fd, ELF_C_READ_MMAP, NULL), dyn_data_size would be set to about 130000, representing all the data between the point at which the dynamic section is found in the core file, and the end of the core file itself (which is around 454KB). We then pass the 130KB buffer to elf64_xlatetom, which if we're fortunate, returns an error because the buffer size is not a multiple of sizeof(Elf64_Dyn), but if we're unfortunate, it treats the whole 130KB buffer as Elf64_Dyn entries and fills xlateto.d_buf with garbage. Similar behavior likely occurs everywhere read_portion() is used: dwfl_segment_report_module.c lines 447-453, 545-546. > 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 = dyn_filesz. Which will then be set to the > actual buffer size being read. dwfl_elf_phdr_memory_callback() may read much more than minread or *buffer_size if the ELF file is already mapped in as described above. > If you are protecting against it becoming bigger, should the check be > changed to: > > if (dyn_data_size != 0 && dyn_data_size < dyn_filesz) > dyn_filesz = dyn_data_size; > I think for the purposes of reading small segments (like PT_DYNAMIC and PT_NOTE), we should ignore *buffer_available altogether. Best, Gavin
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
Awesome, thanks for looking over this. I only have one comment: there's an extra "xlatefrom.d_size = xlatefrom.d_size;" line that should be removed. dwfl_elf_phdr_memory_callback is called from dwfl_link_map_report but if any issues arise, those could be addressed in a separate patch. Best, Gavin On Wed, Nov 30, 2022 at 3:14 PM Mark Wielaard <mark@klomp.org> wrote: > > 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
Hi Gavin, On Thu, 2022-12-01 at 13:13 -0800, Gavin Li wrote: > Awesome, thanks for looking over this. I only have one comment: > there's an extra "xlatefrom.d_size = xlatefrom.d_size;" line that > should be removed. Thanks for spotting that. Odd the compiler didn't warn for that. There is a xlatefrom.d_size = phnum * phentsize; just before this that does the correct assignment. Pushed with that line removed. Cheers, Mark
diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c index 287fc002..08aca0eb 100644 --- a/libdwfl/dwfl_segment_report_module.c +++ b/libdwfl/dwfl_segment_report_module.c @@ -821,12 +821,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;