Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[asan] Custom poisoning on stack is broken by optimizations #100639

Closed
vitalybuka opened this issue Jul 25, 2024 · 0 comments · Fixed by #100773
Closed

[asan] Custom poisoning on stack is broken by optimizations #100639

vitalybuka opened this issue Jul 25, 2024 · 0 comments · Fixed by #100773
Assignees

Comments

@vitalybuka
Copy link
Collaborator

This is reproducer on
https://github.com/abseil/abseil-cpp/blob/c98bd9c8840f9ded87cf1fd1238455468d325628/absl/strings/cord_test.cc

Note: Revision is important as we are planing to land absl:: workaround.

libc++ also hit that issue

https://github.com/llvm/llvm-project/pull/79536/files#diff-534bc2907ddb3b074ded1353d18fd7d578daf1707943b3039bab4ed975aba3b3R772

// This must not be static to avoid aggressive optimizations.
ABSL_ATTRIBUTE_WEAK
size_t FalseReport(const absl::Cord& a, bool f);

ABSL_ATTRIBUTE_NOINLINE
size_t FalseReport(const absl::Cord& a, bool f) {
  absl::Cord b;
  const absl::Cord& ref = f ? b : a;
  // Test that sanitizers report nothing here. Without
  // InlineData::Rep::annotated_this() compiler can unconditionally load
  // poisoned parts, assuming that local variable is fully accessible.
  return ref.size();
}

TEST(CordSanitizerTest, SanitizesCordFalseReport) {
  absl::Cord c;
  for (int i = 0; i < 1000; ++i)
    c.Append("a");
  FalseReport(c, false);
}

I will add minimized repro later.

The issues is in InstCombine or SimplifyCFG, and likely should be fixed with llvm::mustSuppressSpeculation.

@vitalybuka vitalybuka self-assigned this Jul 25, 2024
copybara-service bot pushed a commit to abseil/abseil-cpp that referenced this issue Jul 25, 2024
Many compiler optimizations, even some in Asan, assume that size of
the object on the stack is fixed. Compiler takes into account the lifetime
of the stack object, but it does not expect that size of the object can
change.

This assumption allow to accesses when ever object is alive.

However, this assumption does not work well with custom poisoning
of objects by sanitizers. `Cord` may poison parts of the object
to improve a precision of the bug detection. But compiler still may
load from poisoned parts of the object, assuming it's alive.

So without the patch we can have `false-positive`, because
compiler reordered load from poisoned local before size-check.
llvm/llvm-project#100639

Alternative is to make compiler optimization to know about
possibility of local object resize, but it's not worth of effort,
as it's needed only for sanitizers so far.

Another minor issue fixed with this patch is `false-negative`.
llvm/llvm-project#100640
Asan drops instrumentation of memory accesses proven to be inbound
of local variable. However if we if we take into account `custom
poisoning` we will have to pessimize too often: any random function
call can potentially `resize` a variable by `poisoning`.

We are already using similar `poisoning` workaround in libc++:
https://github.com/llvm/llvm-project/pull/79536/files#diff-534bc2907ddb3b074ded1353d18fd7d578daf1707943b3039bab4ed975aba3b3R772

PiperOrigin-RevId: 656129711
Change-Id: I6d78997da6d31c7ab979a00b84dc9b3b7cffc26f
vitalybuka added a commit that referenced this issue Jul 26, 2024
Even if memory is valid from `llvm` point of view,
e.g. local alloca, sanitizers have API for user
specific memory annotations.

This annotations can be used to track size of the
local object, e.g. inline vector like may prevent
accessed beyond the current vector size.

So valid programs should not access those parts of
alloca before checking preconditions.

Fixes #100639.

Pull Request: #100773
vitalybuka added a commit that referenced this issue Jul 27, 2024
Even if memory is valid from `llvm` point of view,
e.g. local alloca, sanitizers have API for user
specific memory annotations.

This annotations can be used to track size of the
local object, e.g. inline vector like may prevent
accessed beyond the current vector size.

So valid programs should not access those parts of
alloca before checking preconditions.

Fixes #100639.

Pull Request: #100773
vitalybuka added a commit that referenced this issue Jul 29, 2024
And extract `suppressSpeculativeLoadForSanitizers`.

For #100639.
vitalybuka added a commit that referenced this issue Jul 29, 2024
In #100773 we will go conservative for sanitizers,
so it's better to pinpoint location consciously.

For #100639.
vitalybuka added a commit that referenced this issue Jul 29, 2024
…100844)

Some optimization need to be undone with
sanitizers by #100773.

For #100639.
copybara-service bot pushed a commit to abseil/abseil-cpp that referenced this issue Jul 29, 2024
llvm/llvm-project#100639

The second issue is still relevant.

PiperOrigin-RevId: 657298935
Change-Id: Ifa2f1627deee3fc24e23ea78c5b01ed232813390
banach-space pushed a commit to banach-space/llvm-project that referenced this issue Aug 7, 2024
)

And extract `suppressSpeculativeLoadForSanitizers`.

For llvm#100639.
banach-space pushed a commit to banach-space/llvm-project that referenced this issue Aug 7, 2024
In llvm#100773 we will go conservative for sanitizers,
so it's better to pinpoint location consciously.

For llvm#100639.
banach-space pushed a commit to banach-space/llvm-project that referenced this issue Aug 7, 2024
banach-space pushed a commit to banach-space/llvm-project that referenced this issue Aug 7, 2024
…00773)

Even if memory is valid from `llvm` point of view,
e.g. local alloca, sanitizers have API for user
specific memory annotations.

These annotations can be used to track size of the
local object, e.g. inline vectors may prevent
accesses beyond the current vector size.

So valid programs should not access those parts of
alloca before checking preconditions.

Fixes llvm#100639.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants