This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dmikulin on Sep 19 2017, 2:54 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

dmikulin created this revision.Sep 19 2017, 2:54 PM
kcc edited edge metadata.Sep 19 2017, 3:07 PM

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?

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.

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?

-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 updated this revision to Diff 115932.Sep 19 2017, 5:40 PM
dmikulin edited the summary of this revision. (Show Details)

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 added a comment.Sep 20 2017, 4:35 PM

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 added a comment.EditedSep 20 2017, 4:44 PM

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.

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)

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 added a comment.Sep 21 2017, 12:13 PM

Yes, thanks!
What if we poison the metadata?

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

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 added a comment.Sep 21 2017, 5:08 PM

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

Just call PoisonShadow?

eugenis edited edge metadata.Sep 21 2017, 5:12 PM

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

dmikulin updated this revision to Diff 116300.Sep 21 2017, 7:55 PM

Updated the patch. Do it all in runtime.

kcc added a comment.Sep 21 2017, 9:23 PM

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 marked an inline comment as done.Sep 22 2017, 9:50 AM

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 added a comment.Sep 22 2017, 12:49 PM

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

dmikulin updated this revision to Diff 116395.Sep 22 2017, 2:01 PM
dmikulin edited the summary of this revision. (Show Details)

Updated diff.

Can someone please click "Approve"? ;)

eugenis accepted this revision.Sep 26 2017, 4:26 PM
This revision is now accepted and ready to land.Sep 26 2017, 4:26 PM
This revision was automatically updated to reflect the committed changes.