Page MenuHomePhabricator

[asan] Stop instrumenting user-defined ELF sections
ClosedPublic

Authored by mgorny on Mar 23 2020, 8:34 PM.

Details

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.

Diff Detail

Event Timeline

krytarowski created this revision.Mar 23 2020, 8:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2020, 8:34 PM

This is probably missing a test

vitalybuka requested changes to this revision.Apr 8 2020, 6:19 PM
This revision now requires changes to proceed.Apr 8 2020, 6:19 PM
R3x added a subscriber: R3x.May 25 2020, 5:04 PM

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

mgorny added a subscriber: mgorny.May 28 2020, 12:09 PM
mgorny commandeered this revision.Jun 1 2020, 8:31 AM
mgorny updated this revision to Diff 267631.
mgorny edited reviewers, added: krytarowski; removed: mgorny.

Added a test.

pcc added a subscriber: pcc.Jun 1 2020, 10:18 AM
pcc added inline comments.
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 marked an inline comment as done.Jun 1 2020, 11:49 AM
mgorny added inline comments.
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 added inline comments.Jun 2 2020, 5:30 AM
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 added inline comments.Jun 5 2020, 12:05 PM
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;
  }
}
lebedev.ri resigned from this revision.Jun 12 2020, 5:29 AM
mgorny updated this revision to Diff 270713.Jun 15 2020, 3:58 AM
mgorny retitled this revision from [asan] Stop instrumenting NetBSD-specific link_set sections to [asan] Stop instrumenting user-defined ELF sections.
mgorny edited the summary of this revision. (Show Details)

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

glider added a subscriber: glider.Jun 16 2020, 12:47 AM
glider added inline comments.
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1875

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.

vitalybuka resigned from this revision.Jul 6 2020, 9:45 PM
kcc added a comment.Jul 7 2020, 6:49 PM

can we instead slap an attribute on these special variables?

In D76665#2138050, @kcc wrote:

can we instead slap an attribute on these special variables?

Could you explain, please?

kcc added a comment.Jul 8 2020, 2:53 PM

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 added a comment.EditedJul 20 2020, 5:48 AM
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 updated this revision to Diff 279277.Jul 20 2020, 9:25 AM

Here's a version with the extra test.

@glider + @pcc please have a look!

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 added a comment.EditedSep 5 2020, 12:57 PM
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 accepted this revision.Oct 2 2020, 8:23 AM
This revision is now accepted and ready to land.Oct 2 2020, 8:23 AM

Confirmed to work.

This revision was automatically updated to reflect the committed changes.