Partial fix for bug 34607: ASan misses global underflow in first symbol of data section

[dmikulin]

Summary

ASan allocates a global data initialization array at the tail end of each compunit's .data section. This vector is not poisoned. Because of this the first symbol of the following section has no left red zone. As a result, ASan cannot detect underflow for such symbols.

Poison ASan allocated metadata, it should not be accessible to user code.

This fix does not eliminate the problem with missing left red zones but it reduces the set of vulnerable symbols from first symbols in each input data section to first symbols in the output section of the binary.

[kcc]

Please put this under a flag, off by default. (something like -mllvm -asan-globals-separate-section=1)
This is an obvious fix with totally non-obvious consequences, we don't want to find these consequences in the hard way.

Also, why getGlobalMetadataSection?

[dmikulin]

In D38056#875680, @kcc wrote:

Please put this under a flag, off by default. (something like -mllvm -asan-globals-separate-section=1)
This is an obvious fix with totally non-obvious consequences, we don't want to find these consequences in the hard way.

Sure, I can put it under an option. Why do you think this change may have dangerous consequences? We already do something very similar with -fdata-sections.

Also, why getGlobalMetadataSection?

It's already used with -fdata-sections, I figured I'd stick with it for consistency.

[kcc]

In D38056#875690, @dmikulin wrote:

In D38056#875680, @kcc wrote:

Please put this under a flag, off by default. (something like -mllvm -asan-globals-separate-section=1)
This is an obvious fix with totally non-obvious consequences, we don't want to find these consequences in the hard way.

Sure, I can put it under an option. Why do you think this change may have dangerous consequences? We already do something very similar with -fdata-sections.

asan has lots of diverse users. Any change will affect someone.
We'll want to enable this flag gradually and see what happens.

Also, why getGlobalMetadataSection?

It's already used with -fdata-sections, I figured I'd stick with it for consistency.

Hmm.. Maybe add a comment then?
I don't know much about -fdata-sections. Could someone else comment?

[dmikulin]

-fdata-sections places each data symbol into a separate section and all ASan metadata goes into its own special section, like "asan_globals" on Linux.

[dmikulin]

Made the change optional.

I would prefer to enable it by default though. If it's disabled, hardly anyone will use it and we won't know how safe it is. Since I don't have enough exposure to the variety of apps that use ASan, I can't make a definitive argument in favor of having it enabled by default. I understand that changing data layout may cause some programs, arguably not very well behaved, to behave differently, but this change fixes a real set of issues and it does not deviate from the already established method of allocating ASan metadata under -fdata-sections.

[kcc]

I would prefer to enable it by default though

Of course. But we can't do it until we test it on various targets.
I can't *promise* you that we will test it on various targets any time soon, but we may at least try.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

181 ↗

(On Diff #115932)

Why does this say something about metadata?
These are just the asan-ified globals

1884 ↗

(On Diff #115932)

Same here: why is this metadata?

1889 ↗

(On Diff #115932)

I still don't get why the section is used, the comment doesn't help.

[dmikulin]

If you look closely, AllGlobals is not holding Asan-ified globals, it's holding the array of MetadataInitializers. Maybe the previous choice of the variable name wasn't descriptive, I can change it.

[kcc]

I'm still confused...
I'll be a sloppy reviewer in next 2 weeks, adding eugenis, who touched this code recently.
Ad please also add a test in test/Instrumentation/AddressSanitizer/ (with and w/o this flag)

[dmikulin]

This is how .data section is laid out today under ASan:
sym1->RZ1->sym2->RZ2-> ... ->symN->RZN->ASan Metadata Array
The metadata array provides instructions to the runtime on how to poison memory around ASan-ified globals. At runtime red zones are poisoned, but ASan metadata is not. This creates an area at the tail of each compunit data section, which is not poisoned. When sections are concatenated by the linker, it creates unpoisoned holes which should not be accessible to the user. Let's say you have 2 data sections, .data1 followed by .data2, and .data2 has a symbol X at offset 0. Attempts to access memory at &X-1 should be flagged by ASan, but with the current layout they will not be because &X-1 will point into the tail end of the .data1 section where unpoisoned ASan metadata is allocated. Effectively, every symbol at offset 0 in a .data section in each compunit will have no left red zone.
By moving the metadata into a separate section we reduce the number of symbol with no left red zone. All ASan metadata from all objects will be merged by the linker into a single section. The combined .data section will only contain ASan-ified globals.
Hope it clarifies what I'm trying to do with this patch.

[kcc]

Yes, thanks!
What if we poison the metadata?

[dmikulin]

Poisoning the metadata is doable too but I thought it was cleaner to separate user data from metadata.

[dmikulin]

I guess we can do it purely in the runtime code. Initially I thought of allocating another metadata record in the compiler to direct poisoning. But the following does the trick:

diff --git a/compiler-rt/lib/asan/asan_globals.cc b/compiler-rt/lib/asan/asan_globals.cc
index eebada804f0..193b8133477 100644
--- a/compiler-rt/lib/asan/asan_globals.cc
+++ b/compiler-rt/lib/asan/asan_globals.cc
@@ -377,20 +377,22 @@ void __asan_register_globals(__asan_global *globals, uptr n) {
               (sizeof(__asan_global) & (sizeof(__asan_global) - 1)) == 0,
           "sizeof(__asan_global) incompatible with incremental linker padding");
       // If these are padding bytes, the rest of the global should be zero.
       CHECK(globals[i].size == 0 && globals[i].size_with_redzone == 0 &&
             globals[i].name == nullptr && globals[i].module_name == nullptr &&
             globals[i].odr_indicator == 0);
       continue;
     }
     RegisterGlobal(&globals[i]);
   }
+  __asan_global meta = {(uptr) globals, 0, n * sizeof(__asan_global), "__asan_metadata", "__asan", 0, 0, 0};
+  PoisonRedZones(meta);
 }

[kcc]

+ asan_global meta = {(uptr) globals, 0, n * sizeof(asan_global), "asan_metadata", "asan", 0, 0, 0};
+ PoisonRedZones(meta);

Just call PoisonShadow?

[eugenis]

I like the runtime poisoning of metadata arrays. It is simple and very unlikely to break anything.

[dmikulin]

Updated the patch. Do it all in runtime.

[kcc]

Yep, that's better. And we can do it w/o a flag.

compiler-rt/lib/asan/asan_globals.cc

389 ↗

(On Diff #116300)

reinterpret_cast<uptr>(globals)

compiler-rt/test/asan/TestCases/global-underflow.cc

4 ↗

(On Diff #116300)

I assume the test passes with the fix and fails w/o it.
But this test reliable? Will it work with another linker? On another OS?

[dmikulin]

The compiler change from the previous patch is out, so there is no flag.

compiler-rt/test/asan/TestCases/global-underflow.cc

4 ↗

(On Diff #116300)

Yes, it passes with the change and fails without. Tested on MacOS and Linux. This test relies on the section order being the same as the order of object files on the link line. Linkers don't usually re-order sections by default, so I think the assumption should hold. And unless the OS has non-standard alignment requirements for data sections which would create an unpoisoned hole after the first section, it should work on other OSs.

[kcc]

Please address the comment around (uptr) and update the commit message.

[dmikulin]

Updated diff.

[dmikulin]

Can someone please click "Approve"? ;)

compiler-rt/trunk/lib/asan/asan_globals.cc

if (SANITIZER_WINDOWS && globals[i].beg == 0) {
       // If these are padding bytes, the rest of the global should be zero.
       CHECK(globals[i].size == 0 && globals[i].size_with_redzone == 0 &&
             globals[i].name == nullptr && globals[i].module_name == nullptr &&
             globals[i].odr_indicator == 0);
       continue;
     }
     RegisterGlobal(&globals[i]);
   }
+
+  // Poison the metadata. It should not be accessible to user code.
+  PoisonShadow(reinterpret_cast<uptr>(globals), n * sizeof(__asan_global),
+               kAsanGlobalRedzoneMagic);
 }
 
 // Unregister an array of globals.
 // We must do this when a shared objects gets dlclosed.
 void __asan_unregister_globals(__asan_global *globals, uptr n) {
   if (!flags()->report_globals) return;
   BlockingMutexLock lock(&mu_for_globals);
   for (uptr i = 0; i < n; i++) {

compiler-rt/trunk/test/asan/TestCases/Helpers/underflow.cc

+int YYY[3]={1,2,3};

compiler-rt/trunk/test/asan/TestCases/global-underflow.cc

+// RUN: %clangxx_asan -O0 %s %p/Helpers/underflow.cc -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O1 %s %p/Helpers/underflow.cc -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O2 %s %p/Helpers/underflow.cc -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O3 %s %p/Helpers/underflow.cc -o %t && not %run %t 2>&1 | FileCheck %s
+
+int XXX[2] = {2, 3};
+extern int YYY[];
+#include <string.h>
+int main(int argc, char **argv) {
+  memset(XXX, 0, 2*sizeof(int));
+  // CHECK: {{READ of size 4 at 0x.* thread T0}}
+  // CHECK: {{    #0 0x.* in main .*global-underflow.cc:}}[[@LINE+3]]
+  // CHECK: {{0x.* is located 4 bytes to the left of global variable}}
+  // CHECK:   {{.*YYY.* of size 12}}
+  int res = YYY[-1];
+  return res;
+}