[asan] Stop instrumenting user-defined ELF sections

[mgorny]

Summary

Do not instrument user-defined ELF sections (whose names resemble valid
C identifiers). They may have special use semantics and modifying them
may break programs. This is e.g. the case with NetBSD __link_set API
that expects these sections to store consecutive array elements.

[lebedev.ri]

This is probably missing a test

[R3x]

@lebedev.ri Can you point me to where this test needs to be added and an example of the same?

[mgorny]

Added a test.

[pcc]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

1859

What do the names of these sections look like? From the test I presume that they have C identifier names and are enumerated via __start_/__stop_. In that case we should exclude all C identifier named sections from instrumentation (not just link_set*) in order to fix similar bugs involving those sections.

[mgorny]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

1859

They are created via the following macro:

#define	__link_set_make_entry(set, sym)					\
	static void const * const __link_set_##set##_sym_##sym		\
	    __section("link_set_" #set) __used = (const void *)&sym

So the link_set_ name is enforced on our end. Of course, people can do any crazy things they like, so can't tell if anyone else has a similar use case.

[krytarowski]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

1859

@pcc can you suggest how to implement a generalized version of this patch that disables instrumentation of user-defined sections?

[pcc]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

1859

Something like:

if (ELF) {
  for (char C : Section) {
    if (!((C >= 'A' && C <= 'Z') || (C >= 'a' && C <= 'a') || (C >= '0' && C <= '9') || C == '_')))
      return false;
  }
}

[mgorny]

Is this what you had in mind? I'm sorry it took me this long to get to it again.

[glider]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

1887

Drive-by comment: if we don't instrument sections with names resembling valid C identifiers, then there should be a test checking that we instrument a section with a name that's an invalid identifier.

[kcc]

can we instead slap an attribute on these special variables?

[mgorny]

In D76665#2138050, @kcc wrote:

can we instead slap an attribute on these special variables?

Could you explain, please?

[kcc]

Will adding attribute((no_sanitize("address"))) to your global solve the problem you are trying to solve?
(sorry for being too terse last time)

[krytarowski]

In D76665#2140245, @kcc wrote:

Will adding attribute((no_sanitize("address"))) to your global solve the problem you are trying to solve?
(sorry for being too terse last time)

It does not work (last time I checked).

Also GCC does the same thing of not instrumenting user defined sections.

[mgorny]

Here's a version with the extra test.

[krytarowski]

@glider + @pcc please have a look!

[krytarowski]

Ping?

[krytarowski]

Ping?

[lebedev.ri]

In D76665#2161919, @krytarowski wrote:

In D76665#2140245, @kcc wrote:

Will adding attribute((no_sanitize("address"))) to your global solve the problem you are trying to solve?
(sorry for being too terse last time)

It does not work (last time I checked).

Does not work how?

Also GCC does the same thing of not instrumenting user defined sections.

[krytarowski]

In D76665#2237050, @lebedev.ri wrote:

In D76665#2161919, @krytarowski wrote:

In D76665#2140245, @kcc wrote:

Will adding attribute((no_sanitize("address"))) to your global solve the problem you are trying to solve?
(sorry for being too terse last time)

It does not work (last time I checked).

Does not work how?

Also GCC does the same thing of not instrumenting user defined sections.

I cannot get a user defined section uninstrumented without the proposed patch. Elements in such sections are not in consecutive order. This is a known problem and fixed by GCC in a similar way to this patch here.

Also reading the context of this patch, some other platforms have faced the same problem.

[krytarowski]

Confirmed to work.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

bool ModuleAddressSanitizer::shouldInstrumentGlobal(GlobalVariable *G) const {
   // semantics: no duplicates, any, or exact match.
   if (Comdat *C = G->getComdat()) {
     switch (C->getSelectionKind()) {
     case Comdat::Any:
     case Comdat::ExactMatch:
     case Comdat::NoDuplicates:
       break;
     case Comdat::Largest:
     case Comdat::SameSize:
       return false;
     }
   }
 
   if (G->hasSection()) {
     // The kernel uses explicit sections for mostly special global variables
     // that we should not instrument. E.g. the kernel may rely on their layout
     // without redzones, or remove them at link time ("discard.*"), etc.
if (G->hasSection()) {
     // Do not instrument function pointers to initialization and termination
     // routines: dynamic linker will not properly handle redzones.
     if (Section.startswith(".preinit_array") ||
         Section.startswith(".init_array") ||
         Section.startswith(".fini_array")) {
       return false;
     }
 
+    // Do not instrument user-defined sections (with names resembling
+    // valid C identifiers)
+    if (TargetTriple.isOSBinFormatELF()) {
+      if (std::all_of(Section.begin(), Section.end(),
+                      [](char c) { return llvm::isAlnum(c) || c == '_'; }))
+        return false;
+    }
+
     // On COFF, if the section name contains '$', it is highly likely that the
     // user is using section sorting to create an array of globals similar to
     // the way initialization callbacks are registered in .init_array and
     // .CRT$XCU. The ATL also registers things in .ATL$__[azm]. Adding redzones
     // to such globals is counterproductive, because the intent is that they
     // will form an array, and out-of-bounds accesses are expected.
     // See https://github.com/google/sanitizers/issues/305
     // and http://msdn.microsoft.com/en-US/en-en/library/bb918180(v=vs.120).aspx

llvm/test/Instrumentation/AddressSanitizer/do-not-instrument-netbsd-link_set.ll

+; This test checks that NetBSD link_set array elements remain consecutive.
+; RUN: opt < %s -asan -asan-module -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-netbsd"
+
+module asm ".hidden __stop_link_set_test_set"
+
+@data1 = dso_local global i32 1, align 4
+@data2 = dso_local global i32 2, align 4
+@__link_set_test_set_sym_data1 = internal constant i8* bitcast (i32* @data1 to i8*), section "link_set_test_set", align 8
+@__link_set_test_set_sym_data2 = internal constant i8* bitcast (i32* @data2 to i8*), section "link_set_test_set", align 8
+; CHECK: @__link_set_test_set_sym_data1 = internal constant i8*{{.*}}, section "link_set_test_set"
+; CHECK-NEXT: @__link_set_test_set_sym_data2 = internal constant i8*{{.*}}, section "link_set_test_set"

llvm/test/Instrumentation/AddressSanitizer/instrument-section-invalid-c-ident.ll

+; This test checks that sections with names not resembling valid C identifiers
+; are instrumented.
+; RUN: opt < %s -asan -asan-module -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-netbsd"
+
+module asm ".hidden invalid$c$name"
+
+@data1 = dso_local global i32 1, align 4
+@data2 = dso_local global i32 2, align 4
+@__invalid$c$name_sym_data1 = internal constant i8* bitcast (i32* @data1 to i8*), section "invalid$c$name", align 8
+@__invalid$c$name_sym_data2 = internal constant i8* bitcast (i32* @data2 to i8*), section "invalid$c$name", align 8
+; CHECK: @"__invalid$c$name_sym_data1" = internal constant{{.*}}, section "invalid$c$name", comdat
+; CHECK-NEXT: @"__invalid$c$name_sym_data2" = internal constant{{.*}}, section "invalid$c$name", comdat
+; CHECK: @"__asan_global___invalid$c$name_sym_data1"
+; CHECK-NEXT: @"__asan_global___invalid$c$name_sym_data2"