Project

General

Profile

Actions
Copy link

Bug #1231

closed

AddressSanitizer detects an global buffer overflow when we iterate over plugin .so descriptors

Added by Francis Deslauriers almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
libbabeltrace2
Target version:
Babeltrace - stable 2.0
Start date:
02/17/2020
Due date:
% Done:

0%

Estimated time:

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

Download all files
0001-Fix-plugin-dev.h-Disable-address-sanitizer-on-pointe.patch (5.39 KB) 0001-Fix-plugin-dev.h-Disable-address-sanitizer-on-pointe.patch Mathieu Desnoyers, 02/17/2020 06:39 PM
0001-Fix-plugin-dev.h-Disable-address-sanitizer-on-pointe.patch (5.96 KB) 0001-Fix-plugin-dev.h-Disable-address-sanitizer-on-pointe.patch Updated patch (v2) Mathieu Desnoyers, 02/17/2020 08:14 PM
Actions
Copy link
 #1

Updated by Jonathan Rajotte Julien almost 5 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
Actions
Copy link
 #2

Updated by Mathieu Desnoyers almost 5 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.

Actions
Copy link
 #3

Updated by Mathieu Desnoyers almost 5 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.

Actions
Copy link
 #4

Updated by Mathieu Desnoyers almost 5 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.

Actions
Copy link
 #5

Updated by Mathieu Desnoyers almost 5 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.

Actions
Copy link
 #6

Updated by Jonathan Rajotte Julien almost 5 years ago

  • Author changed from 215 to 35
Actions
Copy link
 #7

Updated by Mathieu Desnoyers almost 5 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.

Actions
Copy link
 #8

Updated by Mathieu Desnoyers almost 5 years ago

The attached patch fixes the issue on my setup (Linux). Can you check that it works fine on yours, and on MacOS X ?

Actions
Copy link
 #9

Updated by Mathieu Desnoyers almost 5 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

Actions
Copy link
 #10

Updated by Mathieu Desnoyers almost 5 years ago

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.

Actions
Copy link
 #11

Updated by Jonathan Rajotte Julien almost 5 years ago

Migrated from internal bug tracker.

Actions
Copy link
 #12

Updated by Francis Deslauriers almost 5 years ago

Actions
Copy link
 #13

Updated by Francis Deslauriers almost 5 years ago

  • Status changed from Confirmed to Waiting for review
Actions
Copy link
 #14

Updated by Francis Deslauriers almost 5 years ago

  • Status changed from Waiting for review to Resolved

Merged

Actions
Copy link

Also available in: Atom PDF