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
