[Bug libelf/24081] buffer over-read Problem in elf32_xlatetom function in libelf
https://sourceware.org/bugzilla/show_bug.cgi?id=24081 wcventure changed: What|Removed |Added Summary|Use-After-free Problem in |buffer over-read Problem in |elf32_xlatetom function in |elf32_xlatetom function in |libelf |libelf -- You are receiving this mail because: You are on the CC list for the bug.
[Bug backends/24075] Program Crash due to buffer over-read in ebl_object_note function in eblobjnote.c in libebl.
https://sourceware.org/bugzilla/show_bug.cgi?id=24075 Mark Wielaard changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED --- Comment #7 from Mark Wielaard --- Nice find. The property data is padded and we also must make sure that the extra padding fits the note description. commit cd7ded3df43f655af945c869976401a602e46fcd (HEAD -> master) Author: Mark Wielaard Date: Wed Jan 30 00:04:11 2019 +0100 libebl: Check GNU property note data padding fits inside note. The GNU property note data is padded. Make sure the extra padding still fits in the note description. https://sourceware.org/bugzilla/show_bug.cgi?id=24075 Signed-off-by: Mark Wielaard -- You are receiving this mail because: You are on the CC list for the bug.
[PATCH elfutils] [tests] parse inode in /proc/pid/maps/correctly in run-backtrace-data.sh
The backtrace-data.c parsed the inode in /proc/pid/maps with
format "%*x".
This caused failure if inode is big. For example,
7f269223d000-7f269226b000 r-xp 00:50 10224326387095067468
/home/...
The error likes below:
-bash-4.4$ cat run-backtrace-data.sh.log
backtrace-data:
/home/engshare/elfutils/0.174/src/elfutils-0.174/tests/backtrace-data.c:110:
maps_lookup: Assertion `errno == 0' failed.
/home/engshare/elfutils/0.174/src/elfutils-0.174/tests/test-subr.sh: line 84:
3123578 Aborted (core dumped)
LD_LIBRARY_PATH="${built_library_path}${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH"
$VALGRIND_CMD "$@"
data: no main
-bash-4.4$
The reason is errno is ERANGE.
Fix the test with inode format string "%*u" as inode here is presented
as decimal numbers.
Suggested-by: Mark Wielaard
Signed-off-by: Yonghong Song
---
tests/backtrace-data.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/backtrace-data.c b/tests/backtrace-data.c
index 3a91c664..b389d6af 100644
--- a/tests/backtrace-data.c
+++ b/tests/backtrace-data.c
@@ -106,7 +106,7 @@ maps_lookup (pid_t pid, Dwarf_Addr addr, GElf_Addr *basep)
{
// 37e3c22000-37e3c23000 rw-p 00022000 00:11 49532 /lib64/ld-2.14.90.so
*/
unsigned long start, end, offset;
- i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*x", , , );
+ i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*u", , , );
assert (errno == 0);
if (i != 3)
break;
--
2.17.1
Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh
On 1/29/19 12:50 PM, Mark Wielaard wrote:
> On Fri, Jan 25, 2019 at 01:20:09PM -0800, Yonghong Song wrote:
>> The backtrace-data.c parsed the inode in /proc/pid/maps with
>> format "%*x".
>> This caused failure if inode is big. For example,
>>7f269223d000-7f269226b000 r-xp 00:50 10224326387095067468
>> /home/...
>
> I have a bit of trouble replicating this (with a simple sscanf).
> How exactly does it fail?
The error message looks like:
-bash-4.4$ cat run-backtrace-data.sh.log
backtrace-data:
/home/engshare/elfutils/0.174/src/elfutils-0.174/tests/backtrace-data.c:110:
maps_lookup: Assertion `errno == 0' failed.
/home/engshare/elfutils/0.174/src/elfutils-0.174/tests/test-subr.sh:
line 84: 3123578 Aborted (core dumped)
LD_LIBRARY_PATH="${built_library_path}${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH"
$VALGRIND_CMD "$@"
data: no main
-bash-4.4$
The reason is errno is ERANGE.
>
>> The correct format should be "%*lu" to reflect inode "unsigned long" type.
>> But that caused the following compilation error.
>>acktrace-data.c: In function ‘maps_lookup’:
>>backtrace-data.c:109:22: error: use of assignment suppression and length
>> modifier
>>together in gnu_scanf format [-Werror=format=]
>> i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*lu", , ,
>> );
>
> Not that it matters much, since we are really ignoring the rest of the
> line and this is just a test. But I do wonder why %*u doesn't work.
> The warning says you cannot combine a length specifier with
> a ignored format specifier. Which kind of makes sense given that the
> length is for the variable to assign the value for, not the format.
> So it seems $*u should do the trick. But since I haven't been able
> to make the original fail, I might be wrong.
"%u" works as well. Let me submit another patch for this.
Thanks!
>
> Cheers,
>
> Mark
>
Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh
On Fri, Jan 25, 2019 at 01:20:09PM -0800, Yonghong Song wrote: > The backtrace-data.c parsed the inode in /proc/pid/maps with > format "%*x". > This caused failure if inode is big. For example, > 7f269223d000-7f269226b000 r-xp 00:50 10224326387095067468 > /home/... I have a bit of trouble replicating this (with a simple sscanf). How exactly does it fail? > The correct format should be "%*lu" to reflect inode "unsigned long" type. > But that caused the following compilation error. > acktrace-data.c: In function ‘maps_lookup’: > backtrace-data.c:109:22: error: use of assignment suppression and length > modifier > together in gnu_scanf format [-Werror=format=] >i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*lu", , , ); Not that it matters much, since we are really ignoring the rest of the line and this is just a test. But I do wonder why %*u doesn't work. The warning says you cannot combine a length specifier with a ignored format specifier. Which kind of makes sense given that the length is for the variable to assign the value for, not the format. So it seems $*u should do the trick. But since I haven't been able to make the original fail, I might be wrong. Cheers, Mark
Re: [PATCH elfutils 1/2] [libdwfl] parse inode in /proc/pid/maps correctly
On Fri, Jan 25, 2019 at 01:20:08PM -0800, Yonghong Song wrote: > The inode number in /proc/pid/maps is displayed as "unsigned long" > type. > > In one of our x64 system, we have inode number exceeding valid "long" > type range, which caused the following test failure: >FAIL: dwfl-bug-fd-leak >FAIL: run-backtrace-dwarf.sh >FAIL: vdsosyms > > The offending map entry: > 7f269246b000-7f269246c000 rw-p 0002e000 00:50 10224326387095067468 /home/... > > This patch changed sscanf inode number type from PRIi64 to PRIu64 > and fixed the problem. Reading the inode number as a signed value was indeed odd. Added a ChangeLog entry and pused to master. Thanks, Mark
Re: [PATCH] configure: Add new --enable-install-elfh option.
Hi Ulf,
On Thu, Jan 24, 2019 at 06:53:15PM +0100, Mark Wielaard wrote:
> On Fri, 2019-01-18 at 14:03 +, Ulf Hermann wrote:
> > I think you should also adapt tests/Makefile.am to use our own elf.h
> > in
> > this case. See https://codereview.qt-project.org/#/c/187812/25 for
> > my
> > solution to this.
>
> Yes, it should indeed.
> I used a slightly different solution though.
> It relies on the default include flags already including the srcdirs.
> Does that work for your use case too? (See revised patch attached.)
Please let me know if you have any more comments or suggestions.
I like to push this to master.
> I looked at the features.h tweak also in your patch.
> But I am not comfortable including something like that.
> It feels like it needs a different test (whether we are actually on a
> POSIX system or not). And I am not sure it really should define uid_t,
> gid_t, mode_t, and pid_t. Those normally don't come from features.h
> (they would come from sys/types.h).
Thanks,
Mark
> From 86f9481187bccb78b2533674bb905a0de1a03abf Mon Sep 17 00:00:00 2001
> From: Mark Wielaard
> Date: Fri, 18 Jan 2019 14:18:22 +0100
> Subject: [PATCH] configure: Add new --enable-install-elfh option.
>
> We explicitly test (with system-elf-libelf) that our include headers
> work with the system elf.h header. But it might be helpful to install
> the elf.h file for a private install. Our elf.h header really is just
> a copy of the latest glibc elf.h. But it might be newer and include
> more constants than the system installed elf.h.
>
> Add a new configure option --enable-install-elfh to install elf.h.
> But warn when it is enabled for the default /usr or /usr/local prefix
> because it might clash with the glibc/system elf.h header in that case.
>
> Signed-off-by: Mark Wielaard
> ---
> ChangeLog | 4
> configure.ac | 12
> libelf/ChangeLog | 5 +
> libelf/Makefile.am | 13 ++---
> tests/ChangeLog| 5 +
> tests/Makefile.am | 4 +++-
> 6 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 45418a0..148ce77 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-01-18 Mark Wielaard
> +
> + * configure.ac: Add new --enable-install-elfh.
> +
> 2018-07-04 Ross Burton
>
> * configure.ac: Check for gawk.
> diff --git a/configure.ac b/configure.ac
> index b89b867..7d4e69d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -323,6 +323,11 @@ if test "$use_valgrind" = yes; then
> fi
> AM_CONDITIONAL(USE_VALGRIND, test "$use_valgrind" = yes)
>
> +AC_ARG_ENABLE([install-elfh],
> +AS_HELP_STRING([--enable-install-elfh],[install elf.h in include dir]),
> + [install_elfh=$enableval], [install_elfh=no])
> +AM_CONDITIONAL(INSTALL_ELFH, test "$install_elfh" = yes)
> +
> AM_CONDITIONAL(BUILD_STATIC, [dnl
> test "$use_gprof" = yes -o "$use_gcov" = yes])
>
> @@ -658,6 +663,7 @@ AC_MSG_NOTICE([
>
>NOT RECOMMENDED FEATURES (should all be no)
> Experimental thread safety : ${use_locks}
> +install elf.h : ${install_elfh}
>
>OTHER FEATURES
> Deterministic archives by default : ${default_ar_deterministic}
> @@ -673,3 +679,9 @@ AC_MSG_NOTICE([
> use rpath in tests : ${tests_use_rpath}
> test biarch: ${utrace_cv_cc_biarch}
> ])
> +
> +if test "$install_elfh" = yes; then
> + if test "${prefix}" = "/usr/local" -o "${prefix}" = "/usr"; then
> +AC_MSG_WARN([installing elf.h in ${includedir} might conflict with
> glibc/system elf.h])
> + fi
> +fi
> diff --git a/libelf/ChangeLog b/libelf/ChangeLog
> index 5783f0c..b89e93f 100644
> --- a/libelf/ChangeLog
> +++ b/libelf/ChangeLog
> @@ -1,3 +1,8 @@
> +2019-01-18 Mark Wielaard
> +
> + * Makefile.am (INSTALL_ELFH): Add elf.h to include_HEADERS when
> + defined, otherwise (the default) add elf.h to noinst_HEADERS.
> +
> 2019-01-16 Mark Wielaard
>
> * note_xlate.h (elf_cvt_note): Check n_namesz and n_descsz don't
> diff --git a/libelf/Makefile.am b/libelf/Makefile.am
> index ddaeaa2..d5d63f7 100644
> --- a/libelf/Makefile.am
> +++ b/libelf/Makefile.am
> @@ -39,6 +39,16 @@ noinst_LIBRARIES = libelf_pic.a
> noinst_PROGRAMS = $(noinst_LIBRARIES:_pic.a=.so)
> include_HEADERS = libelf.h gelf.h nlist.h
>
> +noinst_HEADERS = abstract.h common.h exttypes.h gelf_xlate.h libelfP.h \
> + version_xlate.h gnuhash_xlate.h note_xlate.h dl-hash.h \
> + chdr_xlate.h
> +
> +if INSTALL_ELFH
> +include_HEADERS += elf.h
> +else
> +noinst_HEADERS += elf.h
> +endif
> +
> pkginclude_HEADERS = elf-knowledge.h
>
> libelf_a_SOURCES = elf_version.c elf_hash.c elf_error.c elf_fill.c \
> @@ -123,9 +133,6 @@ uninstall: uninstall-am
> rm -f $(DESTDIR)$(libdir)/libelf.so.$(VERSION)
> rm -f $(DESTDIR)$(libdir)/libelf.so
>
> -noinst_HEADERS = elf.h abstract.h common.h exttypes.h gelf_xlate.h libelfP.h
Re: [PATCHv2 2/2] eu-stack: add support for sysroot option
On Tue, Jan 22, 2019 at 10:16:26AM +, Luke Diamand wrote: > Use the dwfl_set_sysroot() function to set the sysroot to be > used when analysing a core: > > e.g. >$ eu-stack --core core --sysroot /path/to/sysroot -e crashing_prog > > Signed-off-by: Luke Diamand This looks perfect (but is missing a ChangeLog entry). Thanks, Mark
Re: [PATCHv2 1/2] libdwfl: specify optional sysroot to search for shared libraries
On Tue, Jan 22, 2019 at 10:16:25AM +, Luke Diamand wrote:
> When searching the list of modules in a core file, if the core was
> generated on a different system to the current one, we need to look
> in a sysroot for the various shared objects.
>
> For example, we might be looking at a core file from an ARM system
> using elfutils running on an x86 host.
>
> This change adds a new function, dwfl_set_sysroot(), which then
> gets used when searching for libraries.
A few comments below.
Also a ChangeLog entry is missing. I added comments for them.
> Signed-off-by: Luke Diamand
> ---
> libdw/libdw.map| 7 ++-
> libdwfl/dwfl_end.c | 1 +
> libdwfl/libdwfl.h | 5 +
> libdwfl/libdwflP.h | 1 +
> libdwfl/link_map.c | 26 --
> 5 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/libdw/libdw.map b/libdw/libdw.map
> index 55482d58..43a9de2e 100644
> --- a/libdw/libdw.map
> +++ b/libdw/libdw.map
> @@ -360,4 +360,9 @@ ELFUTILS_0.173 {
> ELFUTILS_0.175 {
>global:
> dwelf_elf_begin;
> -} ELFUTILS_0.173;
> \ No newline at end of file
> +} ELFUTILS_0.173;
> +
> +ELFUTILS_0.176 {
> + global:
> +dwfl_set_sysroot;
> +} ELFUTILS_0.175;
OK.
* libdw.map (ELFUTILS_0.176): New section, add dwfl_set_rootsysroot.
> diff --git a/libdwfl/dwfl_end.c b/libdwfl/dwfl_end.c
> index 74ee9e07..345d9947 100644
> --- a/libdwfl/dwfl_end.c
> +++ b/libdwfl/dwfl_end.c
> @@ -45,6 +45,7 @@ dwfl_end (Dwfl *dwfl)
>free (dwfl->lookup_addr);
>free (dwfl->lookup_module);
>free (dwfl->lookup_segndx);
> + free (dwfl->sysroot);
>
>Dwfl_Module *next = dwfl->modulelist;
>while (next != NULL)
OK.
* libdwfl_end.c (dwfl_end): Free dwfl->sysroot.
> diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
> index a0c1d357..c11e2f24 100644
> --- a/libdwfl/libdwfl.h
> +++ b/libdwfl/libdwfl.h
> @@ -807,6 +807,11 @@ int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
> bool dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
>__nonnull_attribute__ (1, 2);
>
> +/* Set the sysroot to use when searching for shared libraries. If not
> + specified, search in the system root. */
> +void dwfl_set_sysroot (Dwfl *dwfl, const char *sysroot)
> + __nonnull_attribute__ (1);
This should document that a copy is made of sysroot which
is owned by the library, the passed in string is owned and
can be freed after the call.
It probably should return an int, so failure can be indicated
(you call realpath, which can fail).
What happens if sysroot was already set?
Does setting it to NULL clear it?
* libdwfl.h (dwfl_set_sysroot): New function declaration.
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
> index 941a8b66..db16ab57 100644
> --- a/libdwfl/libdwflP.h
> +++ b/libdwfl/libdwflP.h
> @@ -138,6 +138,7 @@ struct Dwfl
>int lookup_tail_ndx;
>
>struct Dwfl_User_Core *user_core;
> + char *sysroot; /* sysroot, or NULL to search standard system paths */
> };
OK.
* libdwflP.h (struct Dwfl): Add sysroot field.
>
> #define OFFLINE_REDZONE 0x1
> diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
> index 29307c74..cf18c0a2 100644
> --- a/libdwfl/link_map.c
> +++ b/libdwfl/link_map.c
> @@ -34,6 +34,7 @@
> #include
> #include
> #include
> +#include
>
> /* This element is always provided and always has a constant value.
> This makes it an easy thing to scan for to discern the format. */
> @@ -388,8 +389,21 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t
> elfdata,
>if (name != NULL)
> {
> /* This code is mostly inlined dwfl_report_elf. */
> - // XXX hook for sysroot
> - int fd = open (name, O_RDONLY);
> + char *path_name;
> + int rc;
> + const char *sysroot = dwfl->sysroot;
> +
> + /* don't look in the sysroot if the path is already inside the
> sysroot */
> + bool name_in_sysroot = sysroot && (strncmp(name, sysroot,
> strlen(sysroot)) == 0);
> +
> + if (!name_in_sysroot && sysroot)
The && sysroot is now redundant.
> + rc = asprintf(_name, "%s/%s", sysroot, name);
> + else
> + rc = asprintf(_name, "%s", name);
> + if (unlikely(rc == -1))
> + return release_buffer(-1);
Minor optimization.
In theory you could just assign name to path_name here and then...
> +
> + int fd = open (path_name, O_RDONLY);
> if (fd >= 0)
> {
> Elf *elf;
> @@ -471,6 +485,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t
> elfdata,
> close (fd);
> }
> }
> + free(path_name);
Guard this free with if (name_in_sysroot).
No worries if you think that is ugly, it does make for
easier error handling and resource cleanup.
Please use tab plus 2 spaces to indent.
* link_map.c (report_r_debug): Check and use Dwfl sysroot.
> }
>
>if (mod
Re: [PATCHv1 0/2] specify a sysroot to search when examining a core file
Hi Luke, On Sun, Jan 20, 2019 at 03:00:42PM +, Luke Diamand wrote: > Following on from this discussion: > https://sourceware.org/ml/elfutils-devel/2018-q4/msg00224.html > > This patch adds a new API to specify a sysroot, and extends eu-stack to > use it with a new command line option. > > I have been experimenting with this on various ARM-based platforms, > currently using a virt-qemu platform built from buildroot. I like it! Thanks. Some high level comments, before I review the actual code. - It currently only works for resolving path names found in core files. It might make sense to use the same for other variants of dwfl_report_xxx and maybe add a dwfl_get_sysroot () to be used inside the find_elf callback. This can probably be added later though. - eu-stack uses its own argument parser, but other tools, like addr2line, use the argp parser dwfl_standard_argp (). It would be nice to add a --sysroot argument parser there too that would automatically call dwfl_set_sysroot to make them handle --sysroot automagically. - Is there a way to have some small testcase for this? Cheers, Mark
[Bug backends/24075] Program Crash due to buffer over-read in ebl_object_note function in eblobjnote.c in libebl.
https://sourceware.org/bugzilla/show_bug.cgi?id=24075 --- Comment #6 from wcventure --- CVE-2019-7146 -- You are receiving this mail because: You are on the CC list for the bug.
