From: Gavin Li <gavin@matician.com>
To: Mark Wielaard <mark@klomp.org>
Cc: elfutils-devel@sourceware.org
Subject: Re: [PATCH] libdwfl: Read no more than required to parse dynamic sections
Date: Tue, 29 Nov 2022 13:48:42 -0800 [thread overview]
Message-ID: <CACB127+eBsXE49p+m6oE7Yb1u5gmfVPXgfuf+VNR5QDNqNZZog@mail.gmail.com> (raw)
In-Reply-To: <70c0b7df5c6b38492a0655c0ff552453d9dc1f5d.camel@klomp.org>
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
next prev parent reply other threads:[~2022-11-29 21:48 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-29 6:26 gavin
2022-11-29 15:25 ` Mark Wielaard
2022-11-29 21:48 ` Gavin Li [this message]
2022-11-30 23:14 ` Mark Wielaard
2022-12-01 21:13 ` Gavin Li
2022-12-13 16:49 ` Mark Wielaard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACB127+eBsXE49p+m6oE7Yb1u5gmfVPXgfuf+VNR5QDNqNZZog@mail.gmail.com \
--to=gavin@matician.com \
--cc=elfutils-devel@sourceware.org \
--cc=mark@klomp.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).