public inbox for elfutils@sourceware.org
 help / color / mirror / Atom feed
* Fix some .debug checking and gnu hash xlate logic
@ 2023-02-20 15:55 Mark Wielaard
  2023-02-20 15:55 ` [PATCH 1/2] libelf: memmove any extra bytes left by elf_cvt_gnuhash conversion Mark Wielaard
  2023-02-20 15:55 ` [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section Mark Wielaard
  0 siblings, 2 replies; 8+ messages in thread
From: Mark Wielaard @ 2023-02-20 15:55 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Evgeny Vereshchagin, Aleksei Vetrov

Hi,

The last fuzzer found some use (checking) of undefine/uninitialized
data. Either of these two patches will fix that:

 [PATCH 1/2] libelf: memmove any extra bytes left by elf_cvt_gnuhash
 [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section

Note that the bad data wouldn't actually be used, just checked for
validity. But these patches make sure the result is deterministic.

Cheers,

Mark

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] libelf: memmove any extra bytes left by elf_cvt_gnuhash conversion
  2023-02-20 15:55 Fix some .debug checking and gnu hash xlate logic Mark Wielaard
@ 2023-02-20 15:55 ` Mark Wielaard
  2023-02-20 15:55 ` [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section Mark Wielaard
  1 sibling, 0 replies; 8+ messages in thread
From: Mark Wielaard @ 2023-02-20 15:55 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Evgeny Vereshchagin, Aleksei Vetrov, Mark Wielaard

Otherwise some undefined bytes might be left in the buffer. Now they
might still be not useful, but at least they are as defined in the
file.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 ChangeLog              |  4 ++++
 libelf/gnuhash_xlate.h | 12 ++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index d99d837d..55787f64 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2023-02-20  Mark Wielaard  <mark@klomp.org>
+
+	* gnuhash_xlate.h (elf_cvt_gnuhash): memmove any left over bytes.
+
 2023-02-15  Mark Wielaard  <mark@klomp.org>
 
 	* configure.ac: Error out when demangler is enabled, but
diff --git a/libelf/gnuhash_xlate.h b/libelf/gnuhash_xlate.h
index 6faf1136..3a00ae0a 100644
--- a/libelf/gnuhash_xlate.h
+++ b/libelf/gnuhash_xlate.h
@@ -1,5 +1,6 @@
 /* Conversion functions for versioning information.
    Copyright (C) 2006, 2007 Red Hat, Inc.
+   Copyright (C) 2023, Mark J. Wielaard <mark@klomp.org>
    This file is part of elfutils.
    Written by Ulrich Drepper <drepper@redhat.com>, 2006.
 
@@ -36,6 +37,7 @@
 static void
 elf_cvt_gnuhash (void *dest, const void *src, size_t len, int encode)
 {
+  size_t size = len;
   /* The GNU hash table format on 64 bit machines mixes 32 bit and 64 bit
      words.  We must detangle them here.   */
   Elf32_Word *dest32 = dest;
@@ -45,7 +47,7 @@ elf_cvt_gnuhash (void *dest, const void *src, size_t len, int encode)
   for (unsigned int cnt = 0; cnt < 4; ++cnt)
     {
       if (len < 4)
-	return;
+	goto done;
       dest32[cnt] = bswap_32 (src32[cnt]);
       len -= 4;
     }
@@ -58,7 +60,7 @@ elf_cvt_gnuhash (void *dest, const void *src, size_t len, int encode)
   for (unsigned int cnt = 0; cnt < bitmask_words; ++cnt)
     {
       if (len < 8)
-	return;
+	goto done;
       dest64[cnt] = bswap_64 (src64[cnt]);
       len -= 8;
     }
@@ -71,4 +73,10 @@ elf_cvt_gnuhash (void *dest, const void *src, size_t len, int encode)
       *dest32++ = bswap_32 (*src32++);
       len -= 4;
     }
+
+ done:
+  /* If there are any bytes left, we weren't able to convert the
+     partial structures, just copy them over. */
+  if (len > 0)
+    memmove (dest + size - len, src + size - len, len);
 }
-- 
2.39.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section
  2023-02-20 15:55 Fix some .debug checking and gnu hash xlate logic Mark Wielaard
  2023-02-20 15:55 ` [PATCH 1/2] libelf: memmove any extra bytes left by elf_cvt_gnuhash conversion Mark Wielaard
@ 2023-02-20 15:55 ` Mark Wielaard
  2023-02-20 16:02   ` Aleksei Vetrov
  1 sibling, 1 reply; 8+ messages in thread
From: Mark Wielaard @ 2023-02-20 15:55 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Evgeny Vereshchagin, Aleksei Vetrov, Mark Wielaard

.debug sections are raw bytes and don't need conversion even when host
and file have different endian order.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libdw/ChangeLog         | 4 ++++
 libdw/dwarf_begin_elf.c | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index e0cd8f21..5e60f786 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,7 @@
+2023-02-20  Mark Wielaard  <mark@klomp.org>
+
+	* dwarf_begin_elf.c (check_section): Use elf_rawdata.
+
 2023-02-14  Mark Wielaard  <mark@klomp.org>
 
 	* dwarf_getlocation.c (__libdw_intern_expression): Correct check
diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index 1d4bb333..92d76d24 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -1,5 +1,6 @@
 /* Create descriptor from ELF descriptor for processing file.
    Copyright (C) 2002-2011, 2014, 2015, 2017, 2018 Red Hat, Inc.
+   Copyright (C) 2023, Mark J. Wielaard <mark@klomp.org>
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -242,8 +243,8 @@ check_section (Dwarf *result, size_t shstrndx, Elf_Scn *scn, bool inscngrp)
 	}
     }
 
-  /* Get the section data.  */
-  Elf_Data *data = elf_getdata (scn, NULL);
+  /* Get the section data.  Should be raw bytes, no conversion needed.  */
+  Elf_Data *data = elf_rawdata (scn, NULL);
   if (data == NULL)
     goto err;
 
-- 
2.39.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section
  2023-02-20 15:55 ` [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section Mark Wielaard
@ 2023-02-20 16:02   ` Aleksei Vetrov
  2023-02-21  2:28     ` Evgeny Vereshchagin
  0 siblings, 1 reply; 8+ messages in thread
From: Aleksei Vetrov @ 2023-02-20 16:02 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: elfutils-devel, Evgeny Vereshchagin

[-- Attachment #1: Type: text/plain, Size: 281 bytes --]

Hello, Mark

On Mon, Feb 20, 2023 at 3:55 PM Mark Wielaard <mark@klomp.org> wrote:
>
> .debug sections are raw bytes and don't need conversion even when host
> and file have different endian order.

Thank you! I like this patch more for its simplicity, looks good to me.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section
  2023-02-20 16:02   ` Aleksei Vetrov
@ 2023-02-21  2:28     ` Evgeny Vereshchagin
  2023-02-21 11:24       ` Mark Wielaard
  2023-02-21 16:40       ` Aleksei Vetrov
  0 siblings, 2 replies; 8+ messages in thread
From: Evgeny Vereshchagin @ 2023-02-21  2:28 UTC (permalink / raw)
  To: Aleksei Vetrov; +Cc: Mark Wielaard, elfutils-devel

Hi,

On Mon, 20 Feb 2023 at 19:03, Aleksei Vetrov <vvvvvv@google.com> wrote:
> On Mon, Feb 20, 2023 at 3:55 PM Mark Wielaard <mark@klomp.org> wrote:
> >
> > .debug sections are raw bytes and don't need conversion even when host
> > and file have different endian order.
>
> Thank you! I like this patch more for its simplicity, looks good to me.

Agreed. I haven't actually tested the patch though but since it's
covered by the fuzz
target it should be tested once it's merged anyway.

On a somewhat related looking at some recent patches and especially
https://sourceware.org/git/?p=elfutils.git;a=commit;h=64ee2cb792e7b6ba6ad2a5759bff7ce8714e4668
it seems apart from OSS-Fuzz elfutils is fuzzed elsewhere. Aleksei I
wonder if it would
be possible to add those fuzz targets to OSS-Fuzz? There are blind
spots there and I think it would be
really great to start covering at least some of them.

Thanks,
Evgeny Vereshchagin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section
  2023-02-21  2:28     ` Evgeny Vereshchagin
@ 2023-02-21 11:24       ` Mark Wielaard
  2023-02-23 10:20         ` Mark Wielaard
  2023-02-21 16:40       ` Aleksei Vetrov
  1 sibling, 1 reply; 8+ messages in thread
From: Mark Wielaard @ 2023-02-21 11:24 UTC (permalink / raw)
  To: Evgeny Vereshchagin, Aleksei Vetrov; +Cc: elfutils-devel

Hi,

On Tue, 2023-02-21 at 05:28 +0300, Evgeny Vereshchagin wrote:
> On Mon, 20 Feb 2023 at 19:03, Aleksei Vetrov <vvvvvv@google.com> wrote:
> > On Mon, Feb 20, 2023 at 3:55 PM Mark Wielaard <mark@klomp.org> wrote:
> > > 
> > > .debug sections are raw bytes and don't need conversion even when host
> > > and file have different endian order.
> > 
> > Thank you! I like this patch more for its simplicity, looks good to me.
> 
> Agreed. I haven't actually tested the patch though but since it's
> covered by the fuzz
> target it should be tested once it's merged anyway.

I was actually planning on pushing both patches.
This one makes sure the conversion code isn't called, because that is
unnecessary in this case. The first patch adjusts the conversion code
so it doesn't leave some undefined bytes in the section data.

> On a somewhat related looking at some recent patches and especially
> https://sourceware.org/git/?p=elfutils.git;a=commit;h=64ee2cb792e7b6ba6ad2a5759bff7ce8714e4668
> it seems apart from OSS-Fuzz elfutils is fuzzed elsewhere. Aleksei I
> wonder if it would
> be possible to add those fuzz targets to OSS-Fuzz? There are blind
> spots there and I think it would be
> really great to start covering at least some of them.

I do often run a fuzzer (afl with --enable-sanitize-undefined and --
enable-sanitize-address with CC="afl-gcc -m32") when writing a new
testcase. Some testcases are nice as fuzz targets because they test
just one function, so running the fuzzer for a couple of hours exhausts
the different input values.

Cheers,

Mark

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section
  2023-02-21  2:28     ` Evgeny Vereshchagin
  2023-02-21 11:24       ` Mark Wielaard
@ 2023-02-21 16:40       ` Aleksei Vetrov
  1 sibling, 0 replies; 8+ messages in thread
From: Aleksei Vetrov @ 2023-02-21 16:40 UTC (permalink / raw)
  To: Evgeny Vereshchagin; +Cc: Mark Wielaard, elfutils-devel

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

Hi Evgeny,

On Tue, Feb 21, 2023 at 2:29 AM Evgeny Vereshchagin <evverx@gmail.com>
wrote:
> Aleksei I wonder if it would be possible to add those fuzz targets to
> OSS-Fuzz? There are blind spots there and I think it would be really
great to
> start covering at least some of them.

We are fuzzing a tool named STG
(https://android.googlesource.com/platform/external/stg/+/refs/heads/master
),
which is using libdw and libdwfl from elfutils. And it already has support
for
execution through libFuzzer:
https://android.googlesource.com/platform/external/stg/+/refs/heads/master/fuzz/

The problem is in building infrastructure. STG as fuzzing target is built
inside
Google using internal build and fuzzing infrastructure, but in principle it
does
the same thing as OSS-Fuzz. An AOSP version of STG is built using Android
build
system, which doesn't support the same simplicity of building with
libFuzzer and
sanitizers. So it needs some work to integrate STG into OSS-Fuzz.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section
  2023-02-21 11:24       ` Mark Wielaard
@ 2023-02-23 10:20         ` Mark Wielaard
  0 siblings, 0 replies; 8+ messages in thread
From: Mark Wielaard @ 2023-02-23 10:20 UTC (permalink / raw)
  To: Evgeny Vereshchagin, Aleksei Vetrov; +Cc: elfutils-devel

Hi,

On Tue, 2023-02-21 at 12:24 +0100, Mark Wielaard wrote:
> On Tue, 2023-02-21 at 05:28 +0300, Evgeny Vereshchagin wrote:
> > On Mon, 20 Feb 2023 at 19:03, Aleksei Vetrov <vvvvvv@google.com> wrote:
> > > On Mon, Feb 20, 2023 at 3:55 PM Mark Wielaard <mark@klomp.org> wrote:
> > > > 
> > > > .debug sections are raw bytes and don't need conversion even when host
> > > > and file have different endian order.
> > > 
> > > Thank you! I like this patch more for its simplicity, looks good to me.
> > 
> > Agreed. I haven't actually tested the patch though but since it's
> > covered by the fuzz
> > target it should be tested once it's merged anyway.
> 
> I was actually planning on pushing both patches.
> This one makes sure the conversion code isn't called, because that is
> unnecessary in this case. The first patch adjusts the conversion code
> so it doesn't leave some undefined bytes in the section data.

I have pushed both now.

Cheers,

Mark

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-02-23 10:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-20 15:55 Fix some .debug checking and gnu hash xlate logic Mark Wielaard
2023-02-20 15:55 ` [PATCH 1/2] libelf: memmove any extra bytes left by elf_cvt_gnuhash conversion Mark Wielaard
2023-02-20 15:55 ` [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section Mark Wielaard
2023-02-20 16:02   ` Aleksei Vetrov
2023-02-21  2:28     ` Evgeny Vereshchagin
2023-02-21 11:24       ` Mark Wielaard
2023-02-23 10:20         ` Mark Wielaard
2023-02-21 16:40       ` Aleksei Vetrov

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).