Oh, right, please change it to "Copyright (C) 2026 Matej Smycka" then.
Thanks for the review. ne 21. 6. 2026 v 1:45 odesílatel Mark Wielaard <[email protected]> napsal: > Hi Matej, > > On Sat, Jun 20, 2026 at 12:06:50AM +0200, Matej Smycka wrote: > > phoff (e_phoff) comes straight from the untrusted file header, and the > > sum is computed in unsigned 64-bit with no overflow check. > > > > * libdwfl/elf-from-memory.c (elf_from_remote_memory): Use an > > overflow-safe bound when checking whether the phdrs are already > > in the initial read. > > * tests/elf-from-memory.c: New file. > > * tests/Makefile.am (check_PROGRAMS): Add elf-from-memory. > > (TESTS): Likewise. > > (elf_from_memory_LDADD): New variable. > > Thanks for adding a new test. > > > Signed-off-by: Matej Smycka <[email protected]> > > --- > > libdwfl/elf-from-memory.c | 3 +- > > tests/Makefile.am | 6 +- > > tests/elf-from-memory.c | 116 ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 122 insertions(+), 3 deletions(-) > > create mode 100644 tests/elf-from-memory.c > > > > diff --git a/libdwfl/elf-from-memory.c b/libdwfl/elf-from-memory.c > > index 14b2f5c..775d5a1 100644 > > --- a/libdwfl/elf-from-memory.c > > +++ b/libdwfl/elf-from-memory.c > > @@ -170,7 +170,8 @@ elf_from_remote_memory (GElf_Addr ehdr_vma, > > xlatefrom.d_type = xlateto.d_type = ELF_T_PHDR; > > xlatefrom.d_size = phnum * phentsize; > > > > - if ((size_t) nread >= phoff + phnum * phentsize) > > + if (phoff <= (size_t) nread > > + && (size_t) phnum * phentsize <= (size_t) nread - phoff) > > /* We already have all the phdrs from the initial read. */ > > xlatefrom.d_buf = buffer + phoff; > > else > > Right, checking phoff first and then checking if the remainder is > enough for the phdrs is good. We already know that phnum is < 0xffff > and phentsize is sizeof Elf32_Phdr or Elf64_Phdr so there is no > overflow anymore. > > > diff --git a/tests/Makefile.am b/tests/Makefile.am > > index fb0690e..7936ddb 100644 > > --- a/tests/Makefile.am > > +++ b/tests/Makefile.am > > @@ -68,7 +68,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx > scnnames sectiondump \ > > cu-dwp-section-info declfiles test-manyfuncs \ > > eu_search_cfi eu_search_macros \ > > eu_search_lines eu_search_die \ > > - ar-extract-ar \ > > + ar-extract-ar elf-from-memory \ > > $(asm_TESTS) > > > > asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \ > > @@ -238,7 +238,8 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh > newfile test-nlist \ > > run-sysroot.sh \ > > run-test-manyfuncs.sh \ > > run-eu-search-cfi.sh run-eu-search-macros.sh \ > > - run-eu-search-lines.sh run-eu-search-die.sh > > + run-eu-search-lines.sh run-eu-search-die.sh \ > > + elf-from-memory > > > > if !BIARCH > > export ELFUTILS_DISABLE_BIARCH = 1 > > @@ -867,6 +868,7 @@ deleted_lib_so_CFLAGS = $(fpic_CFLAGS) > -fasynchronous-unwind-tables > > aggregate_size_LDADD = $(libdw) $(libelf) $(argp_LDADD) > > peel_type_LDADD = $(libdw) $(libelf) $(argp_LDADD) > > vdsosyms_LDADD = $(libeu) $(libdw) $(libelf) > > +elf_from_memory_LDADD = ../libdw/libdw.a -lz $(zip_LIBS) $(libelf) > $(libebl) $(libeu) -ldl -lpthread > > getsrc_die_LDADD = $(libeu) $(libdw) $(libelf) > > strptr_LDADD = $(libelf) > > newdata_LDADD = $(libelf) > > OK, added new testcase. No shell wrapper. > Links directly to libdw.a (to get at internal symbol). > > > diff --git a/tests/elf-from-memory.c b/tests/elf-from-memory.c > > new file mode 100644 > > index 0000000..2e99157 > > --- /dev/null > > +++ b/tests/elf-from-memory.c > > @@ -0,0 +1,116 @@ > > +/* Test elf_from_remote_memory phdr bounds handling. > > + Copyright (C) 2026 Red Hat, Inc. > > + This file is part of elfutils. > > Is that copyright notice correct? > > > + > > + This file is free software; you can redistribute it and/or modify > > + it under the terms of the GNU General Public License as published by > > + the Free Software Foundation; either version 3 of the License, or > > + (at your option) any later version. > > + > > + elfutils is distributed in the hope that it will be useful, but > > + WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + General Public License for more details. > > + > > + You should have received a copy of the GNU General Public License > > + along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > + > > +#ifdef HAVE_CONFIG_H > > +# include <config.h> > > +#endif > > + > > +#include <elf.h> > > +#include <gelf.h> > > +#include <libelf.h> > > +#include <stdint.h> > > +#include <stdio.h> > > +#include <string.h> > > +#include <stdlib.h> > > + > > +extern Elf *elf_from_remote_memory (GElf_Addr ehdr_vma, GElf_Xword > pagesize, > > + GElf_Addr *loadbasep, > > + ssize_t (*read_memory) (void *, void *, > > + GElf_Addr, > size_t, > > + size_t), > > + void *arg); > > OK, get internal symbol. > > > + > > +#define BACKSZ 256 > > +static unsigned char backing[BACKSZ]; > > + > > +static ssize_t > > +read_mem (void *arg, void *data, GElf_Addr address, size_t minread, > > + size_t maxread) > > +{ > > + (void) arg; > > + if (address >= BACKSZ) > > + return 0; > > + size_t avail = BACKSZ - (size_t) address; > > + size_t n = avail < maxread ? avail : maxread; > > + if (n < minread) > > + return 0; > > + memcpy (data, backing + address, n); > > + return (ssize_t) n; > > +} > > OK read_memory callback. > > > +static void > > +init_ehdr (Elf64_Ehdr *ehdr) > > +{ > > + memset (ehdr, 0, sizeof *ehdr); > > + memcpy (ehdr->e_ident, ELFMAG, SELFMAG); > > + ehdr->e_ident[EI_CLASS] = ELFCLASS64; > > + ehdr->e_ident[EI_DATA] = ELFDATA2LSB; > > + ehdr->e_ident[EI_VERSION] = EV_CURRENT; > > + ehdr->e_type = ET_CORE; > > + ehdr->e_machine = EM_X86_64; > > + ehdr->e_version = EV_CURRENT; > > + ehdr->e_phnum = 1; > > + ehdr->e_phentsize = sizeof (Elf64_Phdr); > > +} > > OK, looks like a valid ehdr. > > > +int > > +main (void) > > +{ > > + Elf64_Ehdr ehdr; > > + int result = 0; > > + > > + elf_version (EV_CURRENT); > > + > > + init_ehdr (&ehdr); > > + ehdr.e_phoff = (Elf64_Off) (0ULL - (uint64_t) sizeof (Elf64_Phdr)); > > Right, silly big. > > > + memset (backing, 0, sizeof backing); > > + memcpy (backing, &ehdr, sizeof ehdr); > > + Elf *elf = elf_from_remote_memory (0, 4096, NULL, read_mem, NULL); > > + if (elf != NULL) > > + { > > + printf ("FAIL: wrapping e_phoff did not fail safely\n"); > > + elf_end (elf); > > + result = 1; > > + } > > + else > > + printf ("PASS: wrapping e_phoff handled without OOB read\n"); > > Good. Fails without your sanity check. > > > + > > + init_ehdr (&ehdr); > > + ehdr.e_phoff = sizeof (Elf64_Ehdr); > > OK, That is a reasonable phoff. > > > + Elf64_Phdr phdr; > > + memset (&phdr, 0, sizeof phdr); > > + phdr.p_type = PT_LOAD; > > + phdr.p_filesz = BACKSZ; > > + phdr.p_memsz = BACKSZ; > > + phdr.p_align = 4096; > > + memset (backing, 0, sizeof backing); > > + memcpy (backing, &ehdr, sizeof ehdr); > > + memcpy (backing + ehdr.e_phoff, &phdr, sizeof phdr); > > And a reasonable phdr. > > > + elf = elf_from_remote_memory (0, 4096, NULL, read_mem, NULL); > > + GElf_Ehdr got; > > + if (elf == NULL || gelf_getehdr (elf, &got) == NULL || got.e_phnum != > 1) > > + { > > + printf ("FAIL: legitimate image rejected\n"); > > + result = 1; > > + } > > + else > > + printf ("PASS: legitimate image accepted\n"); > > Nice sanity checks. > > > + if (elf != NULL) > > + elf_end (elf); > > And cleanup. > > > + return result; > > +} > > All looks good. > Let me know about the Copyright line and I'll push. > > Thanks, > > Mark > -- *S pozdravem Matěj Smyčka*
