From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1033.google.com (mail-pj1-x1033.google.com [IPv6:2607:f8b0:4864:20::1033]) by sourceware.org (Postfix) with ESMTPS id 0BC0C3858C83 for ; Tue, 29 Nov 2022 21:48:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0BC0C3858C83 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-pj1-x1033.google.com with SMTP id 3-20020a17090a098300b00219041dcbe9so33734pjo.3 for ; Tue, 29 Nov 2022 13:48:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=matician-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=ziOSllMG2/ovGExnB09XqwwE3XP3EkqXbBVvQJG/1xE=; b=qyF0VeVGSFdo7DPwqOgv2CxGEgIfC3DI/XFgt47i4jis1BZiKp1umOYmdCwVlI/h8a DjPDER5V0EAhQ96BVmG+eeV1kEdOXTktDudvUrwJPusgSuOzmoev6n/2KqZ1x7noZ3Hv 5lSfnT9uRL4rXvhvhHWx6b0YkADEHp9/A/4815R3TUNYYtgOc66kSzih9wfbw7RKuTjM puR6eGOAcfMwzbHdsTmMjRxmRNk9r4SGpXyC574le7X00SFQkDpOJSVvCE+BHWfvSDlk TaZwexqPwT5rNTrd8wubsBPxoRB0UpcZX7vgQGNhRtcIzBVxw3mN0enWt41XyCUkRLQl URzA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ziOSllMG2/ovGExnB09XqwwE3XP3EkqXbBVvQJG/1xE=; b=2DaKW3StNLyqPepPJJL5fZOdx/Y97V78AC48wTPygxEk64ZeDK2azr4oq9qFk8/blq S9JPGnZs3t9FHhmkX+i24CEUguAN1IlEAEcoVXWBANj+A78qCWTGVA9V+gKzKQqMZTsz ToVRCmGKdtOBHk04ndY7ejkfAhH8sGsZZNo85Xeq6sA4KU9LDvFQEm3vvtbFUWzfFhJ2 3hJc5xelGVvHolouRMZmVFwPYH0rADYDc0a0XnpDEUDF+31S6BQrbqaNCoMeAYYv7mn4 gOywZrkZd9DaXqSh+IMfVsFAa0yIp3LilPgC8GY2/gJF0xQCKe1xoU1CdjeZYK4fZmtp UGNg== X-Gm-Message-State: ANoB5pl39+rxP3nKS1VOoI43GT1F0jXOcxadmgrJBAewaN6LzqHOghR/ 6g3ec0beHDTD+h8QgatIque7D6g9cF5lebZIGijHLAH2DTkTOg== X-Google-Smtp-Source: AA0mqf4EsdKw5LmyhizV11/2gUyCsrDFEFcQjOsffqaVxXSwXRpXlVvMP/6YbTjbnBjpuM3YgiqsFwprryr6bwq3iGQ= X-Received: by 2002:a17:90a:a591:b0:219:2926:372a with SMTP id b17-20020a17090aa59100b002192926372amr16345764pjq.110.1669758533900; Tue, 29 Nov 2022 13:48:53 -0800 (PST) MIME-Version: 1.0 References: <20221129062653.298772-1-gavin@matician.com> <70c0b7df5c6b38492a0655c0ff552453d9dc1f5d.camel@klomp.org> In-Reply-To: <70c0b7df5c6b38492a0655c0ff552453d9dc1f5d.camel@klomp.org> From: Gavin Li Date: Tue, 29 Nov 2022 13:48:42 -0800 Message-ID: Subject: Re: [PATCH] libdwfl: Read no more than required to parse dynamic sections To: Mark Wielaard Cc: elfutils-devel@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,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 List-Id: 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