Page MenuHomePhabricator

[StackProtector] Catch direct out-of-bounds when checking address-takenness
ClosedPublic

Authored by john.brawn on Mar 5 2020, 9:53 AM.

Details

Summary

With -fstack-protector-strong we check if a non-array variable has its address taken in a way that could cause a potential out-of-bounds access. However what we don't catch is when the address is directly used to create an out-of-bounds memory access.

Fix this by examining the offsets of GEPs that are ultimately derived from allocas and checking if the resulting address is out-of-bounds, and by checking that any memory operations using such addresses are not over-large.

Fixes PR43478.

Diff Detail

Event Timeline

john.brawn created this revision.Mar 5 2020, 9:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2020, 9:53 AM
efriedma added inline comments.
llvm/lib/CodeGen/StackProtector.cpp
210

I think AllocaInst has a getAllocatedType, or something like that?

Should use getTypeAllocSize, since that's what actually determines the number of bytes allocated.

john.brawn marked an inline comment as done.Mar 6 2020, 5:19 AM
john.brawn added inline comments.
llvm/lib/CodeGen/StackProtector.cpp
210

I think AllocaInst has a getAllocatedType, or something like that?

AI isn't necessarily an AllocaInst, e.g. we could be looking at a GEP of a GEP or a GEP of a PHI.

Should use getTypeAllocSize, since that's what actually determines the number of bytes allocated.

Will do.

john.brawn updated this revision to Diff 248710.Mar 6 2020, 5:20 AM

Use getTypeAllocSize, apply clang-format.

I have to admit I didn't thoroughly look at the test, although I have to wonder if the loop cases in the test really provide any additional coverage? They're using phis as input, but you already have phi cases.

llvm/lib/CodeGen/StackProtector.cpp
202

potentially out-of-bounds

203

s/would also have to be/could also be/

llvm/test/CodeGen/Generic/stack-guard-oob.ll
6 ↗(On Diff #248710)

REQUIRES: aarch64-registered-target
REQUIRES: arm-registered-target
REQUIRES: x86-registered-target

7 ↗(On Diff #248710)

When CHECK-LABEL specifies an actual label, it's best to include the appropriate punctuation; so, CHECK-LABEL: in_bounds: and so on throughout.

john.brawn marked 5 inline comments as done.Mar 9 2020, 10:01 AM
john.brawn added inline comments.
llvm/test/CodeGen/Generic/stack-guard-oob.ll
6 ↗(On Diff #248710)

I think it makes more sense instead to split this into three separate tests (which just use the same input file in test/Codegen/Inputs, like we do with stack-guard-reassign.ll), so we don't need to requiring all targets if we want to test any of them.

john.brawn marked an inline comment as done.

Update based on review comments.

Changes look good, but there's still the question about whether you really need the loop tests.

Changes look good, but there's still the question about whether you really need the loop tests.

I think I originally added these because of a previous attempt at a fix, which was more complicated (in a way that didn't improve anything) and did at one point have a problem with loops. I could remove them, though I don't see much reason to. I will if you really want though.

Changes look good, but there's still the question about whether you really need the loop tests.

I think I originally added these because of a previous attempt at a fix, which was more complicated (in a way that didn't improve anything) and did at one point have a problem with loops. I could remove them, though I don't see much reason to. I will if you really want though.

Project practice is to try to have regression tests that are fairly focused, and don't contain any extraneous or redundant cases. This helps future devs trying to read and understand your test. It also reduces testing time by an admittedly small increment, but when you add up those increments across 10s of Ks of tests and N-hundred developers running them many many times per day, it adds up.
There's a place for more thorough testing, IMO, but that place is not the lit tests.

Removed loop tests.

efriedma added inline comments.Mar 10 2020, 3:28 PM
llvm/lib/CodeGen/StackProtector.cpp
210

Oh, I see, the function recurses.

I don't think the recursion works correctly in general. The values that actually matter are the current offset, determined recursively, the size of the original alloca, and the size of any memory operations using the result of the GEP. Using the size of the GEP's operand type doesn't work correctly in general; in particular, if there's another GEP or bitcast in the recursive chain of uses, you can come up with the wrong result.

john.brawn marked an inline comment as done.Mar 11 2020, 6:55 AM
john.brawn added inline comments.
llvm/lib/CodeGen/StackProtector.cpp
210

I think adding a check that bitcast doesn't bitcast to a larger type should be enough to handle this. If we have a GEP of a GEP then each one will make sure that the offset is within the bounds of the specific type it's handling. Doing it like this does mean that for something like

int fn() {
  int arr[4][4];
  return arr[0][5];
}

we say it's an out-of-bounds access when it doesn't actually access outside of the underlying object, but I don't think handling this kind of case is worth the added complexity.

Added bitcast handling.

arsenm requested changes to this revision.Mar 11 2020, 7:29 AM
arsenm added inline comments.
llvm/lib/CodeGen/StackProtector.cpp
220–221

This is introducing new dependences on the deprecated pointee type. No decisions should be made based on this

This revision now requires changes to proceed.Mar 11 2020, 7:29 AM
john.brawn marked an inline comment as done.Mar 11 2020, 9:04 AM
john.brawn added inline comments.
llvm/lib/CodeGen/StackProtector.cpp
220–221

I don't see anything in Type.h about getPointerElementType being deprecated, but looking at Instructions.h I see mention of "opaque pointer types", is this what you're talking about? Looking at http://lists.llvm.org/pipermail/llvm-dev/2019-December/137684.html (which is the best I could find for what it means) it looks like instead of

%var = alloca i32, align 4
%bitcast = i32* %var to %i64
store i64 0, i32* %bitcast

we'll have

%var = alloca i32, align 4
store i64 0, p0 %var

So it looks like I should be checking the type at the load/store instead?

arsenm added inline comments.Mar 11 2020, 9:06 AM
llvm/lib/CodeGen/StackProtector.cpp
220–221

Yes, pointer types should be assumed to be opaque now. Only the type on the use instruction is meaningful

efriedma added inline comments.Mar 11 2020, 10:12 AM
llvm/lib/CodeGen/StackProtector.cpp
210

That's still not enough given the way you're doing the check. Consider if someone writes something like

struct X { int a[10][10]; int b; };
struct X x;
int (*p)[10] = &x.a[10];
int z = a[5];

This lowers to two GEPs; neither is "out-of-bounds" in the sense you're checking.

You could try to enforce that each index of each GEP is in the valid range, I guess.

john.brawn edited the summary of this revision. (Show Details)

Adjusted to avoid looking through pointer types by passing around the allocated type and checking it against the size of memory accesses. Also correctly handle GEPs where the addressed member is partially outside of the allocated object.

I think it would be more straightforward to reason about the behavior here if you just pass down the number of bytes that are known safe to dereference, instead of "AllocType".

Use the allocation size instead of the allocation type.

efriedma added inline comments.Mar 13 2020, 2:30 PM
llvm/lib/CodeGen/StackProtector.cpp
209

Can we just compute ResultSize as "AllocSize - Offset"? (That's obviously correct, and I'm not convinced that using the size of getResultElementType() does the right thing here.)

john.brawn marked an inline comment as done.Mar 16 2020, 6:37 AM
john.brawn added inline comments.
llvm/lib/CodeGen/StackProtector.cpp
209

I think we can. Currently the offset check is done in a way that also checks if the end of the addressed member is past the end of the allocation, but just checking that the start is within the allocation and then reducing the remaining size by the offset will mean we check at the time we see a memory access that it's not too large for the remaining space.

Adjust GEP handling to reduce AllocSize by Offset instead of using the result size.

efriedma accepted this revision.Mar 16 2020, 11:44 AM

I don't think we're really getting much benefit out of testing this on multiple targets; as long as we have coverage for 32-bit and 64-bit targets, none of the relevant logic is actually target-specific. So I'd just get rid of the ARM/AArch64 tests, I think, since x86 conveniently has 32-bit and 64-bit targets.

Otherwise LGTM

This revision was not accepted when it landed; it landed in state Needs Review.Mar 17 2020, 5:43 AM
This revision was automatically updated to reflect the committed changes.