hwasan: globals which have been assigned a section cannot be instrumented

[shuffle2]

llvm-project/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

Lines 1782 to 1786 in 0a68a9d

	// Globals with custom sections may be used in __start_/__stop_ enumeration,
	// which would be broken both by adding tags and potentially by the extra
	// padding/alignment that we insert.
	if (GV.hasSection())
	continue;

I'm not really sure what possible scenario this could be referring to, however there should be a way to bypass this check and allow all globals to be instrumented.

I'm using llvm's hwasan instrumentation with a custom runtime for embedded (baremetal) environment, where assigning sections of code/data symbols is heavily used. It is desirable to instrument all globals and work around any issues which arise from that.

[shuffle2]

possible fix?

diff --git a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
index 75a19357ea1b..c33152023893 100644
--- a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
@@ -1818,10 +1818,9 @@ void HWAddressSanitizer::instrumentGlobals() {
     if (GV.hasCommonLinkage())
       continue;

-    // Globals with custom sections may be used in __start_/__stop_ enumeration,
-    // which would be broken both by adding tags and potentially by the extra
-    // padding/alignment that we insert.
-    if (GV.hasSection())
+    // Avoid adding metadata emitted for the hwasan instrumentation itself.
+    auto section = GV.getSection();
+    if (section == "hwasan_globals" || section == ".note.hwasan.globals")
       continue;

     Globals.push_back(&GV);

it seems the purpose of the existing behavior is just to skip over symbols which are allocated into hwasan-sepecific sections, which logic is all within the same file...

ping @pcc , who wrote this originally

[pcc]

No, it's because users can do

__attribute__((section("foo"))) int myglobal1;
__attribute__((section("foo"))) int myglobal2;

and use __start_foo/__stop_foo to enumerate myglobal1 and myglobal2.

We may want to add an attribute, say __attribute__((sanitize("hwaddress"))), which can be used to enable HWASan on globals. Then you can do something like

#define MYSECTION __attribute__((section("mysection"), sanitize("hwaddress")))

and use MYSECTION to mark your globals.

[shuffle2]

Thanks for the quick response.

What's the existing behavior with hwasan and -fdata-sections, or just using __start_<some_standardized_section_name> ? I guess that it's not possible to reference the symbol from C since the name contains a period in those cases?

How about adding the behavior in the above patch behind a cmdline flag? Or possibly, adding the above behavior as default and reverting to old behavior with a cmdline flag...

IMO enabling hwasan should default to making hwasan work. Omitting instrumentation of globals should be opt-in behavior.
The reasoning seems a bit backwards to me: if a user is relying on __start_foo/__end_foo to iterate some symbols, then they can certainly modify that code to account for size/alignment differences when building with HWASAN enabled. Such code is already fragile and requiring tight integration with linker and final binary layout. However, globals will be allocated into the same section whether hwasan is enabled or not, and it's not expected for there to be such an issue.

At least, there should be a diagnostic that instrumentation is being skipped for user-defined globals. Currently, it causes a silent false-negative condition as the globals can never be instrumented. In codebases I'm dealing with, this is normally the majority of globals (and certainly all the "interesting" ones, which may be buggy).

Adding another attribute is workable, but seems like the most invasive way to fix this among the other options.

[shuffle2]

Another thing to note is the current documentation: https://llvm.org/docs/LangRef.html#global-attributes

no_sanitize_hwaddress
This attribute indicates that the global variable should not have HWAddressSanitizer instrumentation applied to it, because it was annotated with attribute((no_sanitize(“hwaddress”))), attribute((disable_sanitizer_instrumentation)), or included in the -fsanitize-ignorelist file.

so it would seem the actual current behavior is inverse to this, where the doc describes opt-out behavior. Similarly for the other sanitizers (like memtag): they are default-on with opt-out. (I guess this is arguable since it's for llvm lang and not C, but it would be nice to be uniform in approach :D )

[pcc]

cc @fmayer

What's the existing behavior with hwasan and -fdata-sections, or just using _start<some_standardized_section_name> ? I guess that it's not possible to reference the symbol from C since the name contains a period in those cases?

For the former the instrumentation is on because that doesn't involve adding a section attribute. For the latter, the symbol isn't defined (see isValidCIdentifier in lld/ELF), but there are other cases besides start/stop symbols where we need the instrumentation to be turned off (see the comments in shouldTagGlobal mentioned below).

How about adding the behavior in the above patch behind a cmdline flag? Or possibly, adding the above behavior as default and reverting to old behavior with a cmdline flag...

I don't think we can change the default behavior. Maybe a flag would work? But since "whether it will work to enable instrumentation for this global" is a property of the source code, I feel like it should be a source code annotation.

IMO enabling hwasan should default to making hwasan work. Omitting instrumentation of globals should be opt-in behavior.

You can't have both of these things unfortunately. "Making hwasan work" also means not breaking programs that use start/stop symbols or similar features, and we can't have that without omitting the instrumentation for the globals with custom sections.

The reasoning seems a bit backwards to me: if a user is relying on __start_foo/__end_foo to iterate some symbols, then they can certainly modify that code to account for size/alignment differences when building with HWASAN enabled.

That doesn't work because it's not just size/alignment, the user doesn't know which tag was chosen for each global so their accesses to the globals will fail because they will almost certainly be using the wrong tag. Even if that were made to work somehow, requiring users to modify code like this goes against the general principle that hwasan should work on unmodified code.

At least, there should be a diagnostic that instrumentation is being skipped for user-defined globals. Currently, it causes a silent false-negative condition as the globals can never be instrumented. In codebases I'm dealing with, this is normally the majority of globals (and certainly all the "interesting" ones, which may be buggy).

Hmm, maybe? But I'm not sure whether warning for each of these would have a good signal/noise ratio (it could be useful if it helps you realize that all of your globals are uninstrumented, but not so much if only a few are, because you only lose a bit of coverage, and bugs involving globals are much less common than heap/stack). So there would need to be a way to silence the warning, for example with an explicit __attribute__((no_sanitize("hwaddress"))).

Adding another attribute is workable, but seems like the most invasive way to fix this among the other options.

Defaulting to instrumentation would have also been invasive for users of start/stop symbols, especially since it would have caused their valid code to crash, while in your case, at least heap/stack instrumentation works. The use of section attributes for start/stop symbols isn't that uncommon in userspace programs which is the primary use case for HWASan, so it seems reasonable to cater for the more common use case by default.

so it would seem the actual current behavior is inverse to this, where the doc describes opt-out behavior. Similarly for the other sanitizers (like memtag): they are default-on with opt-out.

That documentation doesn't mean that there are no other circumstances where instrumentation is disabled. MTE is also disabled on globals with custom sections for the same reason (see shouldTagGlobal in llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp).

[shuffle2]

That doesn't work because it's not just size/alignment, the user doesn't know which tag was chosen for each global so their accesses to the globals will fail because they will almost certainly be using the wrong tag.

Another approach could be to allow the user to mark these pointers which intentionally straddle multiple globals with the hwasan-match-all-tag value (and ensure hwasan-match-all-tag is enabled to some valid value). Maybe llvm could do it automatically for such symbols? This seems more proper, since requiring annotation of code abusing undefined behavior shouldn't be a surprise. It's plausible that user code can/should already be doing this, given that the user will choose the match-all-tag and can use the hwasan runtime to untag/retag a pointer.

Yet another approach could be only omitting instrumentation for those globals which reside in a section for which the __start_/__end_ symbols are actually referenced. This would also allow a diagnostic to be more useful, since it could only apply to symbols omitted because of such conflict. However, there's the issue of the user defining their own linker symbols and using them to iterate globals*.

Defaulting to instrumentation would have also been invasive for users of start/stop symbols, especially since it would have caused their valid code to crash,

is it valid? Walking a pointer between unique objects does not sound like valid / well-defined C (in fact, it's not: https://port70.net/~nsz/c/c11/n1570.html#6.5.6p8 ). I'm aware it works fine in practice (in fact, I do the same thing to process the hwasan_globals array in my runtime), but my argument is that such code is known to be fragile and should especially not be given favor compared to well-defined, normal code.

while in your case, at least heap/stack instrumentation works.

For most of the code, there is no heap at all, and most globals don't get instrumented. So the effectiveness is questionable.

Hopefully that underlines where I'm coming from a little more and why I feel the existing behavior is backwards.

In any case, I'm eager to get some resolution of this upstreamed in llvm 20.x or 21.x. Currently I've added hwasan-all-globals flag to enable instrumentation of all globals not in hwasan_globals nor .note.hwasan.globals, although would be happy to work on a different solution.

That documentation doesn't mean that there are no other circumstances where instrumentation is disabled. MTE is also disabled on globals with custom sections for the same reason (see shouldTagGlobal in llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp).

Thanks...I'd like to get MTE working eventually too (hw permitting).

here's an example of the UB causing miscompilation (too bad it's not detected by -fsanitize=undefined...): https://gcc.godbolt.org/z/WfhfPYWzo
the original/working version is here: https://gcc.godbolt.org/z/6bdTv6che

[*] actually, now that I think of it, this does seem to call the current rationale into question: https://godbolt.org/z/dfeGreo6K
It's possible for a user to create their own symbols and use them to iterate over globals, even for globals which do not have a custom section assigned. So NO globals are "safe" from this issue. I've probably even written some such code before in real projects...

[pcc]

Another approach could be to allow the user to mark these pointers which intentionally straddle multiple globals with the hwasan-match-all-tag value (and ensure hwasan-match-all-tag is enabled to some valid value). Maybe llvm could do it automatically for such symbols? This seems more proper, since requiring annotation of code abusing undefined behavior shouldn't be a surprise. It's plausible that user code can/should already be doing this, given that the user will choose the match-all-tag and can use the hwasan runtime to untag/retag a pointer.

That wouldn't take care of size/alignment requirements, and wouldn't help with things like .init_array/.ctors/.dtors which are read by the dynamic loader which won't be built with (or know anything about) HWASan.

Yet another approach could be only omitting instrumentation for those globals which reside in a section for which the _start/_end symbols are actually referenced. This would also allow a diagnostic to be more useful, since it could only apply to symbols omitted because of such conflict. However, there's the issue of the user defining their own linker symbols and using them to iterate globals*.

The problem is that the start/stop symbols are typically defined in a separate translation unit to the sections so we don't have enough information to do that. If we were doing LTO it would be a different matter since we could have the linker tell the optimizer which globals have start/stop symbols surrounding them (or other constructs like linker script-defined symbols), but HWASan does not require LTO.

is it valid? Walking a pointer between unique objects does not sound like valid / well-defined C (in fact, it's not: https://port70.net/~nsz/c/c11/n1570.html#6.5.6p8 ). I'm aware it works fine in practice (in fact, I do the same thing to process the hwasan_globals array in my runtime), but my argument is that such code is known to be fragile and should especially not be given favor compared to well-defined, normal code.

Right, it's clearly not well-defined according to the C standard, what I meant was that it's valid in the sense that it correctly uses ELF linker semantics. Adding section attributes to functions is also outside of the C standard, so we're already outside of standards territory here. I think that what we should do when encountering such non-standard constructs is to be pragmatic and do whatever is least likely to break existing code.

For most of the code, there is no heap at all, and most globals don't get instrumented. So the effectiveness is questionable.

Right, but I'm sure you can acknowledge that yours is an unusual case.

here's an example of the UB causing miscompilation (too bad it's not detected by -fsanitize=undefined...): https://gcc.godbolt.org/z/WfhfPYWzo

Hmm, I think we're meant to consider symbols defined like that to alias with everything, so that seems like a bug to me.

[*] actually, now that I think of it, this does seem to call the current rationale into question: https://godbolt.org/z/dfeGreo6K
It's possible for a user to create their own symbols and use them to iterate over globals, even for globals which do not have a custom section assigned. So NO globals are "safe" from this issue. I've probably even written some such code before in real projects...

That's also a case where users are going outside of the standard: the standard doesn't say anything about linker scripts or what the meaning of symbols defined by them are. This is another case where we are pragmatic: if we excluded globals from instrumentation just because there could be a linker script doing something unusual, that would mean that we would end up not instrumenting any globals at all. So we make the assumption that is true >99% of the time that there is either no linker script or that the linker script isn't doing anything like that.