Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Implement -fsanitize=bounds-strict #44349

Open Quuxplusone opened 4 years ago

Quuxplusone commented 4 years ago
Bugzilla Link PR45379
Status NEW
Importance P enhancement
Reported by Kees Cook (keescook@chromium.org)
Reported on 2020-03-31 14:04:19 -0700
Last modified on 2020-03-31 14:36:29 -0700
Version unspecified
Hardware PC Linux
CC efriedma@quicinc.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments bounds.c (2345 bytes, text/x-csrc)
Blocks
Blocked by
See also
Created attachment 23300
bounds checking PoC

I'd like Clang's UBSan bounds checker to disallow the old-style 0-byte and 1-
byte array declarations as being seen as a variant of a flexible array member.

As it turns out, GCC has "-fsanitize=bounds-strict" which gets me close to what
I'd like (it still allows 0-byte):

(Built with gcc -fsanitize=bounds)
$ ./bounds-gcc abc
flex (should always be okay): ok (no trap!)
zero (should be okay, treated as flex): ok (no trap!)
one (should fail): FAIL (this should have trapped!)
non_flex (should fail): FAIL (this should have trapped!)
non_trailing (absolutely should fail): Illegal instruction (core dumped)

(Built with gcc -fsanitize=bounds-strict)
$ ./bounds-gcc abc
flex (should always be okay): ok (no trap!)
zero (should be okay, treated as flex): ok (no trap!)
one (should fail): Illegal instruction (core dumped)

(Built with clang -fsanitize=bounds)
$ ./bounds-clang abc
flex (should always be okay): ok (no trap!)
zero (should be okay, treated as flex): ok (no trap!)
one (should fail): FAIL (this should have trapped!)
non_flex (should fail): Illegal instruction (core dumped)

Could Clang's UBSan grow the "strict" version of this to check 1-byte arrays?
Quuxplusone commented 4 years ago

Attached bounds.c (2345 bytes, text/x-csrc): bounds checking PoC

Quuxplusone commented 4 years ago

If you want to forbid arrays of length 0 at compile-time, you can use -Wzero-length-array. Not sure it's worth adding an option to check at runtime; zero-length arrays are non-standard anyway.

Adding strict checks for arrays of length 1 makes sense, I think.

(The code that performs the check in question is isFlexibleArrayMemberExpr in clang/lib/CodeGen/CGExpr.cpp)