Bug #1231
closedAddressSanitizer detects an global buffer overflow when we iterate over plugin .so descriptors
0%
Description
The code assumes that the descriptors are stored at contiguous addresses in memory and ASan sees those accesses as going out of bound.
I found a similar issue here: https://github.com/google/sanitizers/issues/355
Someone in this thread claims that
Nothing in the C/C++ standard guarantees that the globals will be emitted the way this code expects.
The question is: Are we sure this is safe?
To reproduce this issue simply compile BT with -fsanitize=address as such:
./configure CFLAGS="-fsanitize=address" && make
Here is the output of ASan when running babeltrace path/to/trace
:
================================================================= ==7765==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7ffff2ff18c8 at pc 0x7ffff7966663 bp 0x7fffffffd6e0 sp 0x7fffffffd6d8 READ of size 8 at 0x7ffff2ff18c8 thread T0 #0 0x7ffff7966662 (/usr/local/lib/libbabeltrace.so.2+0x175662) #1 0x7ffff7962aba (/usr/local/lib/libbabeltrace.so.2+0x171aba) #2 0x7ffff7964ca4 (/usr/local/lib/libbabeltrace.so.2+0x173ca4) #3 0x7ffff795b15b (/usr/local/lib/libbabeltrace.so.2+0x16a15b) #4 0x7ffff796167c (/usr/local/lib/libbabeltrace.so.2+0x17067c) #5 0x7ffff620af13 (/lib/x86_64-linux-gnu/libc.so.6+0x111f13) #6 0x7ffff620b3ba (/lib/x86_64-linux-gnu/libc.so.6+0x1123ba) #7 0x7ffff620bbeb (/lib/x86_64-linux-gnu/libc.so.6+0x112beb) #8 0x7ffff795de10 (/usr/local/lib/libbabeltrace.so.2+0x16ce10) #9 0x7ffff795d069 (/usr/local/lib/libbabeltrace.so.2+0x16c069) #10 0x520bbb (/usr/local/bin/babeltrace+0x520bbb) #11 0x517c7b (/usr/local/bin/babeltrace+0x517c7b) #12 0x5160be (/usr/local/bin/babeltrace+0x5160be) #13 0x7ffff611ab96 (/lib/x86_64-linux-gnu/libc.so.6+0x21b96) #14 0x41d2c9 (/usr/local/bin/babeltrace+0x41d2c9) 0x7ffff2ff18c8 is located 56 bytes to the left of global variable '__bt_plugin_descriptor_auto_ptr' defined in 'plugin.c:31:1' (0x7ffff2ff1900) of size 8 0x7ffff2ff18c8 is located 0 bytes to the right of global variable '__bt_plugin_descriptor_dummy' defined in 'plugin.c:28:1' (0x7ffff2ff18c0) of size 8 SUMMARY: AddressSanitizer: global-buffer-overflow (/usr/local/lib/libbabeltrace.so.2+0x175662) Shadow bytes around the buggy address: 0x10007e5f62c0: f9 f9 f9 f9 02 f9 f9 f9 f9 f9 f9 f9 05 f9 f9 f9 0x10007e5f62d0: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9 0x10007e5f62e0: f9 f9 f9 f9 06 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9 0x10007e5f62f0: f9 f9 f9 f9 06 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9 0x10007e5f6300: f9 f9 f9 f9 05 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9 =>0x10007e5f6310: f9 f9 f9 f9 00 00 00 00 00[f9]f9 f9 f9 f9 f9 f9 0x10007e5f6320: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 0x10007e5f6330: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 0x10007e5f6340: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 0x10007e5f6350: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 0x10007e5f6360: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==7765==ABORTING
A more explicit stack trace:
#0 0x00000000004e5b54 in __asan::ReportGenericError(unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long, unsigned int, bool) () #1 0x00000000004e6628 in __asan_report_load8 () #2 0x00007ffff79666e3 in count_non_null_items_in_section (begin=0x7ffff2ff18c0 <__bt_plugin_descriptor_dummy>, end=0x7ffff2ff1940 <__bt_plugin_descriptor_attribute_dummy>) at plugin-so.c:1266 #3 0x00007ffff7962b3b in bt_plugin_so_create_all_from_sections (shared_lib_handle=0x607000000170, descriptors_begin=0x7ffff2ff18c0 <__bt_plugin_descriptor_dummy>, descriptors_end=0x7ffff2ff1940 <__bt_plugin_descriptor_attribute_dummy>, attrs_begin=0x7ffff2ff1940 <__bt_plugin_descriptor_attribute_dummy>, attrs_end=0x7ffff2ff1a40 <__bt_plugin_component_class_descriptor_dummy>, cc_descriptors_begin=0x7ffff2ff1a40 <__bt_plugin_component_class_descriptor_dummy>, cc_descriptors_end=0x7ffff2ff1b00 <__bt_plugin_component_class_descriptor_attribute_dummy>, cc_descr_attrs_begin=0x7ffff2ff1b00 <__bt_plugin_component_class_descriptor_attribute_dummy>, cc_descr_attrs_end=0x7ffff2ff1e00) at plugin-so.c:1293 #4 0x00007ffff7964d25 in bt_plugin_so_create_all_from_file (path=0x621000003d00 "/usr/local/lib/babeltrace/plugins/babeltrace-plugin-text.so") at plugin-so.c:1585 #5 0x00007ffff795b1dc in bt_plugin_find_all_from_file (path=0x621000003d00 "/usr/local/lib/babeltrace/plugins/babeltrace-plugin-text.so") at plugin.c:141 #6 0x00007ffff79616fd in nftw_append_all_from_dir (file=0x621000003d00 "/usr/local/lib/babeltrace/plugins/babeltrace-plugin-text.so", sb=0x7fffffffde70, flag=0, s=0x7fffffffe058) at plugin.c:351 #7 0x00007ffff620af14 in process_entry (data=data@entry=0x7fffffffe030, dir=dir@entry=0x7fffffffdf70, name=name@entry=0x62d0000004b3 "babeltrace-plugin-text.so", namlen=<optimized out>, d_type=8) at ../sysdeps/wordsize-64/../../io/ftw.c:464 #8 0x00007ffff620b3bb in ftw_dir (data=data@entry=0x7fffffffe030, st=st@entry=0x7fffffffe090, old_dir=old_dir@entry=0x0) at ../sysdeps/wordsize-64/../../io/ftw.c:543 #9 0x00007ffff620bbec in ftw_startup (dir=<optimized out>, is_nftw=<optimized out>, func=<optimized out>, descriptors=<optimized out>, flags=1) at ../sysdeps/wordsize-64/../../io/ftw.c:768 #10 0x00007ffff795de91 in bt_plugin_create_append_all_from_dir (plugin_set=0x606000002780, path=0x606000002300 "/usr/local/lib/babeltrace/plugins", recurse=0) at plugin.c:400 #11 0x00007ffff795d0ea in bt_plugin_find_all_from_dir (path=0x606000002300 "/usr/local/lib/babeltrace/plugins", recurse=0) at plugin.c:426 #12 0x0000000000520bbc in load_dynamic_plugins (plugin_paths=0x606000001640) at babeltrace.c:826 #13 0x0000000000517c7c in load_all_plugins (plugin_paths=0x606000001640) at babeltrace.c:865 #14 0x00000000005160bf in main (argc=3, argv=0x7fffffffe588) at babeltrace.c:2950
Files
Updated by Jonathan Rajotte Julien over 4 years ago
Follow-up:
From Simon Marchi:
I managed reproduce it locally using the build line you sent me: ./bootstrap && BABELTRACE_DEV_MODE=1 BABELTRACE_DEBUG_MODE=1 CC="ccache clang-9" CFLAGS="-g -O0 -Werror=incompatible-pointer-types -Werror=int-conversion -Werror=implicit-function-declaration -Wall -fsanitize=address" LDFLAGS="-fsanitize=address -fno-omit-frame-pointer" ./configure --disable-man-pages && make clean && make -j6
Updated by Mathieu Desnoyers over 4 years ago
This is why the Linux kernel and lttng-ust tracepoints don't directly create those sections as "arrays of structure elements", but rather as an array of pointers to structures.
Nothing prevents the compiler from adding padding to the structure elements, but an array of pointers (4 or 8 bytes, as far as I recall basically anything below the size of function text alignment) is fine. I'm not saying the C standard says anything about it (or the opposite), but just that if this breaks, there is a lot of code that will break out there.
Updated by Mathieu Desnoyers over 4 years ago
Actually, bt2 seems to do the same thing as tracepoints, namely use an array of pointers. So we might want to look into adding some kind of ASan annotations to both lttng-ust, the Linux kernel tracepoints (if useful) and bt2 plugin sections.
Updated by Mathieu Desnoyers over 4 years ago
Maybe we could look into disabling asan for the getter functions ?
e.g.: https://github.com/google/sanitizers/wiki/AddressSanitizer
"Turning off instrumentation"
could be applied to every "get" function defined within the BT_PLUGIN_MODULE() macro in include/babeltrace2/plugin/plugin-dev.h
Else we may have to ensure users of asan put the BT_PLUGIN_MODULE() in a separate .o file and disable asan for it.
Updated by Mathieu Desnoyers over 4 years ago
Hrm actually, it would not be the getters per se for which we should disable instrumentation, but rather e.g. count_non_null_items_in_section and friends.
But if asan puts data there which is anything else than a NULL pointer, this could introduce issues.
Updated by Jonathan Rajotte Julien over 4 years ago
- Author changed from 215 to 35
Updated by Mathieu Desnoyers over 4 years ago
Even better:
See https://clang.llvm.org/docs/AddressSanitizer.html
"Disabling Instrumentation with attribute((no_sanitize("address")))
Some code should not be instrumented by AddressSanitizer. One may use the attribute attribute((no_sanitize("address"))) (which has deprecated synonyms no_sanitize_address and no_address_safety_analysis) to disable instrumentation of a particular function. This attribute may not be supported by other compilers, so we suggest to use it together with __has_feature(address_sanitizer).
The same attribute used on a global variable prevents AddressSanitizer from adding redzones around it and detecting out of bounds accesses."
The last part about using it on global variables to remove redzones is what we would want.
Updated by Mathieu Desnoyers over 4 years ago
- File 0001-Fix-plugin-dev.h-Disable-address-sanitizer-on-pointe.patch 0001-Fix-plugin-dev.h-Disable-address-sanitizer-on-pointe.patch added
The attached patch fixes the issue on my setup (Linux). Can you check that it works fine on yours, and on MacOS X ?
Updated by Mathieu Desnoyers over 4 years ago
I'll need to update the patch to properly support C++ as well, which ends up using a different attribute name for this:
http://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html
Updated by Mathieu Desnoyers over 4 years ago
- File 0001-Fix-plugin-dev.h-Disable-address-sanitizer-on-pointe.patch 0001-Fix-plugin-dev.h-Disable-address-sanitizer-on-pointe.patch added
Updated patch. Only targets clang, because gcc does not appear to be affected by the issue (tested on gcc-8), and gcc ignores the no_sanitize applied to global variables.
Updated by Jonathan Rajotte Julien over 4 years ago
Migrated from internal bug tracker.
Updated by Francis Deslauriers over 4 years ago
Gerrit change: https://review.lttng.org/c/babeltrace/+/3102
Updated by Francis Deslauriers over 4 years ago
- Status changed from Confirmed to Waiting for review
Updated by Francis Deslauriers over 4 years ago
- Status changed from Waiting for review to Resolved
Merged