Fix some .debug checking and gnu hash xlate logic

Fix some .debug checking and gnu hash xlate logic

[Mark Wielaard]

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

[PATCH 1/2] libelf: memmove any extra bytes left by elf_cvt_gnuhash conversion

[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

[PATCH 2/2] libdw: Use elf_rawdata when checking .debug section

[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

Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section

[Aleksei Vetrov]

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

Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section

[Evgeny Vereshchagin]

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

Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section

[Mark Wielaard]

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

Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section

[Aleksei Vetrov]

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

Re: [PATCH 2/2] libdw: Use elf_rawdata when checking .debug section

[Mark Wielaard]

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