Re: oss-fuzz
Hi everyone! On Thu, Dec 26, 2019 at 11:50:48PM +0100, Mark Wielaard wrote: Hi Berkeley, On Mon, Dec 23, 2019 at 08:06:54AM +0200, Berkeley Churchill wrote: Great, thanks for the feedback! One of my first tasks will be to support llvm/clang builds. I've seen some prior discussion on what's needed for that, but if you have any extra tips I'll take them. I'll be sure to create a build target for the fuzzers so they can be run standalone. clang is slightly inconvenient because it doesn't implement various GNU C extensions. We even have a configure check for them now so it is clear what we require from a C/gnu99 compiler: https://sourceware.org/git/?p=elfutils.git;a=blob;f=configure.ac;hb=HEAD#l98 In theory when clang support that, everything should just compile. There have been some attempts to rewrite some source code to get clang to accept it: https://sourceware.org/git/?p=elfutils.git&a=search&h=HEAD&st=commit&s=clang But there is just too much code clang simply doesn't parse. I don't know how much work there is left to get clang to accept everything. But Matthias (CCed) said he got somewhat further on irc once. Maybe he can share his patches. The version of elfutils that I maintain is incomplete, but sufficient to run libabigail on top of it and to compile everything with clang. So, my local modifications can be summarized as: - I use a different build system (similar to Bazel (https://bazel.build/) - build a completely static version - disable some warnings to build with -Werror otherwise - Use a static config.h that is created with ./configure --without-lzma --without-bzlib and modified to - #undef HAVE_FALLTHROUGH - #undef HAVE_GCC_STRUCT The source code itself is not modified for building with clang. Though I adopted some patches from the DTS branch to build a version with builtin ebl backends. A comparable series is merged upstream for the next release as far as I can see. The most tricky to address are the nested function definitions. It is not entirely clear to me how to resolve these without completely messing up the existing code. Most of them would end up as macro or factored out function that takes a lot of arguments. I did not find the time to work on a solution that is suitable upstream. A simpler approach would be to see if oss-fuzz really needs clang at all. As far as I know the only thing needed is a compiler supporting inserting tracing calls into every basic block and/or comparison operations and linking to some (C++) library that intercepts those. gcc can do that with -fsanitize-coverage=trace-pc and/or -fsanitize-coverage=trace-cmp (which I believe is command line compatible with what clang uses). I run a fuzzing project against libabigail which indirectly also fuzzes some parts of elfutils (the subset I am compiling). So far, I did not come across a finding in elfutils. But again, I am covering only a very small subset of the code. Cheers, Matthias Cheers, Mark On Mon, Dec 23, 2019 at 3:12 AM Mark Wielaard wrote: > Hi Berkeley, > > On Fri, 2019-12-20 at 17:21 +0200, Berkeley Churchill wrote: > > Any interest in integrating with oss-fuzz? It's a google project > > that supports open source projects by fuzzing. It allows Google to > > find and report bugs, especially security bugs, to the project. > > I'm willing to work on writing fuzzers and performing the integration, > > if this would be welcome by the maintainers. Thoughts? > > Certainly interested. I have been running afl-fuzz on various utilities > and test cases. That has found lots of issues. But it isn't very > structured. And it often needs to go through a completely valid ELF > file before fuzzing the more interesting data structures inside it. > > The only request I would have is that if the fuzzer targets are added > to elfutils itself then they should also be made to work locally. So > someone could also use them with e.g. afl-fuzz or some other fuzzing > framework, or simply as extra testcase. > > Please also see: > https://sourceware.org/git/?p=elfutils.git;f=CONTRIBUTING;hb=HEAD > > Cheers, > > Mark >
[PATCH] libelf: decompress: ensure zlib resource cleanup
__libelf_decompress would only cleanup zlib resources via inflateEnd()
in case inflating was successful, but would leak memory if not. Fix this
by calling inflateEnd() in the error case as well.
Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
Signed-off-by: Matthias Maennich
---
libelf/elf_compress.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index 244467b5e3ae..beb1834bbbd7 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -257,6 +257,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t
size_out)
if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
{
free (buf_out);
+ inflateEnd(&z);
__libelf_seterrno (ELF_E_DECOMPRESS_ERROR);
return NULL;
}
--
2.25.1.481.gfbce0eb801-goog
Re: [PATCH] libelf: decompress: ensure zlib resource cleanup
Hi Mark!
Thanks for the quick response!
On Mon, Mar 16, 2020 at 12:10:51AM +0100, Mark Wielaard wrote:
Hi Matthias,
On Sun, 2020-03-15 at 23:03 +0100, Matthias Maennich via Elfutils-devel wrote:
__libelf_decompress would only cleanup zlib resources via inflateEnd()
in case inflating was successful, but would leak memory if not. Fix this
by calling inflateEnd() in the error case as well.
Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
Signed-off-by: Matthias Maennich
---
libelf/elf_compress.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index 244467b5e3ae..beb1834bbbd7 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -257,6 +257,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t
size_out)
if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
{
free (buf_out);
+ inflateEnd(&z);
__libelf_seterrno (ELF_E_DECOMPRESS_ERROR);
return NULL;
}
This looks correct at first sight, but...
Just before this hunk we do:
if (likely (zrc == Z_OK))
zrc = inflateEnd (&z);
So, zrc can be !Z_OK because the earlier inflateEnd() failed, which
might cause it to call inflateEnd() twice (which might be fine, I
dunno). Should we maybe ignore the error if inflateEnd() and just call
it unconditionally before (ignoring its return code)?
So, replace:
if (... Z_OK) zrc = inflateEnd (&z);
with unconditionally ending the stream:
(void)inflateEnd(&z);
I prefer your variant (and it was my first version of the patch)
independently from what comes below.
Having said that: I looked up what inflateEnd() does and the worst that
could happen is returning an error that we anyway ignore. So, duplicate
calls are not an issue. Also, for the compression part we call
deflateEnd() via a macro in the same duplicate fashion. Hence I
consistently used the same pattern for inflateEnd(). And last, I wanted
to keep that existing error handling. OTOH, projects (including the
example code of zlib [1]) usually just unconditionally call inflateEnd()
ignoring any error codes. So, your call :-)
Cheers,
Matthias
[1] https://zlib.net/zlib_how.html
What do you think?
Cheers,
Mark
[PATCH v2] libelf: {de,}compress: ensure zlib resource cleanup
__libelf_decompress would only cleanup zlib resources via inflateEnd()
in case inflating was successful, but would leak memory if not. Fix this
by calling inflateEnd() unconditionally.
__libelf_decompress did this all the time already, but called
deflateEnd() twice. That is not a (known) issue, but can be cleaned up
by ensuring all error paths use 'return deflate_cleanup' and the success
path calls deflateEnd() only once. Note, the deflate() needs to return
Z_STREAM_END to indicate we are done. Hence change the condition.
Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
Signed-off-by: Matthias Maennich
---
libelf/elf_compress.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index 244467b5e3ae..b1b896890ff7 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -115,7 +115,7 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
{
free (out_buf);
__libelf_seterrno (ELF_E_COMPRESS_ERROR);
- return NULL;
+ return deflate_cleanup(NULL, NULL);
}
Elf_Data cdata;
@@ -197,13 +197,13 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int
ei_data,
}
while (flush != Z_FINISH); /* More data blocks. */
- zrc = deflateEnd (&z);
- if (zrc != Z_OK)
+ if (zrc != Z_STREAM_END)
{
__libelf_seterrno (ELF_E_COMPRESS_ERROR);
return deflate_cleanup (NULL, NULL);
}
+ deflateEnd (&z);
*new_size = used;
return out_buf;
}
@@ -251,16 +251,15 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t
size_out)
}
zrc = inflateReset (&z);
}
- if (likely (zrc == Z_OK))
-zrc = inflateEnd (&z);
if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
{
free (buf_out);
+ buf_out = NULL;
__libelf_seterrno (ELF_E_DECOMPRESS_ERROR);
- return NULL;
}
+ inflateEnd(&z);
return buf_out;
}
--
2.25.1.696.g5e7596f4ac-goog
Re: [PATCH v2] libelf: {de,}compress: ensure zlib resource cleanup
On Sat, Apr 25, 2020 at 01:28:33AM +0200, Mark Wielaard wrote:
Hi,
On Fri, Mar 20, 2020 at 12:17:55PM +0100, Matthias Maennich via Elfutils-devel
wrote:
__libelf_decompress would only cleanup zlib resources via inflateEnd()
in case inflating was successful, but would leak memory if not. Fix this
by calling inflateEnd() unconditionally.
__libelf_decompress did this all the time already, but called
deflateEnd() twice. That is not a (known) issue, but can be cleaned up
by ensuring all error paths use 'return deflate_cleanup' and the success
path calls deflateEnd() only once. Note, the deflate() needs to return
Z_STREAM_END to indicate we are done. Hence change the condition.
Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
Signed-off-by: Matthias Maennich
---
libelf/elf_compress.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index 244467b5e3ae..b1b896890ff7 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -115,7 +115,7 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
{
free (out_buf);
__libelf_seterrno (ELF_E_COMPRESS_ERROR);
- return NULL;
+ return deflate_cleanup(NULL, NULL);
}
I was sure this was correct. But we both missed that deflate_cleanup
is a macro that passes out_buf and frees it. So now it is freed
twice... Oops.
GCC10 (not released yet, but already in Fedora 32 beta) has a new
-fanalyzer option which does catch this:
elf_compress.c: In function ‘__libelf_compress’:
elf_compress.c:50:3: error: double-‘free’ of ‘out_buf’ [CWE-415]
[-Werror=analyzer-double-free]
50 | free (out_buf);
| ^~
‘__libelf_compress’: events 1-10
|
| 50 | free (out_buf);
| | ~~
| | |
| | (10) second ‘free’ here; first ‘free’ was at (9)
|..
| 79 | if (data == NULL)
| | ^
| | |
| | (1) following ‘false’ branch (when ‘data’ is non-NULL)...
|..
| 86 | Elf_Data *next_data = elf_getdata (scn, data);
| |
| | |
| | (2) ...to here
|..
| 91 | *orig_addralign = data->d_align;
| | ~
| | |
| | (3) allocated here
|..
| 100 | if (out_buf == NULL)
| | ~
| | |
| | (4) assuming ‘out_buf’ is non-NULL
| | (5) following ‘false’ branch (when ‘out_buf’ is non-NULL)...
|..
| 107 | size_t used = hsize;
| | ~~
| | |
| | (6) ...to here
|..
| 114 | if (zrc != Z_OK)
| | ~
| | |
| | (7) following ‘true’ branch (when ‘zrc != 0’)...
| 115 | {
| 116 | free (out_buf);
| |
| | |
| | (8) ...to here
| | (9) first ‘free’ here
|
Fixed by removing the free (out_buf) on line 116 as attached.
Hi Mark!
Thanks for catching and fixing that!
Cheers,
Mark
From 0b2fc95c46dabf85d053b2f0c6aab217b9c5a9b8 Mon Sep 17 00:00:00 2001
From: Mark Wielaard
Date: Sat, 25 Apr 2020 01:21:12 +0200
Subject: [PATCH] libelf: Fix double free in __libelf_compress on error path.
In commit 2092865a7e589ff805caa47e69ac9630f34d4f2a
"libelf: {de,}compress: ensure zlib resource cleanup" we added a
call to deflate_cleanup to make sure all resources were freed.
As GCC10 -fanalyzer points out that could cause a double free
of out_buf. Fix by removing the free (out_buf) in __libelf_compress.
Signed-off-by: Mark Wielaard
---
libelf/ChangeLog | 4
libelf/elf_compress.c | 1 -
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 8f79a625..56f5354c 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,7 @@
+2020-04-25 Mark Wielaard
+
+ * elf_compress.c (__libelf_compress): Remove free (out_buf).
+
2020-03-18 Omar Sandoval
* elf_getphdrnum.c (__elf_getphdrnum_rdlock): Call
diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index b1b89689..e5d3d2e0 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -113,7 +113,6 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
int zrc = deflateInit (&z, Z_BEST_COMPRESSION);
if (zrc != Z_OK)
{
- free (out_buf);
Maybe add a comment to the deflate_cleanup macro call then?
Cheers,
Matthias
__libelf_seterrno (ELF_E_COMPRESS_ERROR);
return deflate_cleanup(NULL, NULL);
}
--
2.26.0
[PATCH] dwfl: fix potential overflow when reporting on kernel modules
dwfl_linux_kernel_report_modules_ has an outstanding ancient bug when reading kernel module information from a modules list file. The target buffer for the module name was sized too small to hold potential values. Fix that by increasing the value to account for the null termination. In practice, this unlikely ever happened, but it now got diagnosed by LLVM as part of a stricter -Wfortify-source implementation [1]: libdwfl/linux-kernel-modules.c:1019:7: error: 'sscanf' may overflow; destination buffer in argument 3 has size 128, but the corresponding specifier may require size 129 [-Werror,-Wfortify-source] modname, &modsz, &modaddr) == 3) [1] https://github.com/llvm/llvm-project/commit/2db66f8d48beeea835cb9a6940e25bc04ab5d941 Suggested-by: Paul Pluzhnikov Signed-off-by: Matthias Maennich Change-Id: I97b4fcb536273e5ccc4e37b0b9f0f8ffb7487909 --- libdwfl/linux-kernel-modules.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libdwfl/linux-kernel-modules.c b/libdwfl/linux-kernel-modules.c index c0f8dfa4aef2..58c0c417a597 100644 --- a/libdwfl/linux-kernel-modules.c +++ b/libdwfl/linux-kernel-modules.c @@ -1008,7 +1008,7 @@ dwfl_linux_kernel_report_modules (Dwfl *dwfl) int result = 0; Dwarf_Addr modaddr; unsigned long int modsz; - char modname[128]; + char modname[128+1]; char *line = NULL; size_t linesz = 0; /* We can't just use fscanf here because it's not easy to distinguish \n -- 2.34.0.rc2.393.gf8c9666880-goog
