From 74519e7aa58a19c0ed352b660f2e732148ae2f8a Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Thu, 6 Jun 2019 17:54:00 -0700 Subject: [PATCH 1/3] fortify: fix overflow checks in unistd We should only be calling _real versions of the functions that use this if the input size is verifiably <= SSIZE_MAX. Otherwise, just fall through to _chk and let that handle it. Bug: 131861088 Test: mma && bionic-unit-tests Change-Id: Iba04e486ef91ea1b3539ab6df6260429264e66b4 --- libc/include/bits/fortify/unistd.h | 3 ++- tests/clang_fortify_tests.cpp | 3 --- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/libc/include/bits/fortify/unistd.h b/libc/include/bits/fortify/unistd.h index 543c3c748..547a1686c 100644 --- a/libc/include/bits/fortify/unistd.h +++ b/libc/include/bits/fortify/unistd.h @@ -67,7 +67,8 @@ ssize_t __readlinkat_chk(int dirfd, const char*, char*, size_t, size_t) __INTROD "in call to '" #fn "', '" #what "' bytes overflows the given object") #define __bos_trivially_not_lt_no_overflow(bos_val, index) \ - __bos_dynamic_check_impl_and((bos_val), >=, (index), (bos_val) <= SSIZE_MAX) + ((__bos_dynamic_check_impl_and((bos_val), >=, (index), (bos_val) <= SSIZE_MAX) && \ + __builtin_constant_p(index) && (index) <= SSIZE_MAX)) #if __ANDROID_API__ >= __ANDROID_API_N__ __BIONIC_FORTIFY_INLINE diff --git a/tests/clang_fortify_tests.cpp b/tests/clang_fortify_tests.cpp index e1ecfa508..5f8709830 100644 --- a/tests/clang_fortify_tests.cpp +++ b/tests/clang_fortify_tests.cpp @@ -591,8 +591,6 @@ FORTIFY_TEST(unistd) { EXPECT_FORTIFY_DEATH_STRUCT(getcwd(split.tiny_buffer, sizeof(split))); { - // FIXME: These should all die in FORTIFY. Headers are bugged. -#ifdef COMPILATION_TESTS char* volatile unknown = small_buffer; const size_t count = static_cast(SSIZE_MAX) + 1; // expected-error@+1{{'count' must be <= SSIZE_MAX}} @@ -607,7 +605,6 @@ FORTIFY_TEST(unistd) { EXPECT_FORTIFY_DEATH(pwrite(kBogusFD, unknown, count, 0)); // expected-error@+1{{'count' must be <= SSIZE_MAX}} EXPECT_FORTIFY_DEATH(pwrite64(kBogusFD, unknown, count, 0)); -#endif } } From 849c0b9f51b65da9f3ac7a3222da9ae00893acfd Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Mon, 10 Jun 2019 16:22:09 -0700 Subject: [PATCH 2/3] fortify: add __mempcpy_chk Bug: 131861088 Test: mma + bionic-unit-tests on blueline Change-Id: I02f8f87d5db0ba5fecec410da32f6ffa2c98ef57 --- libc/bionic/fortify.cpp | 7 +++++++ libc/include/bits/fortify/string.h | 16 ++++++++++++++++ libc/libc.map.txt | 1 + tests/clang_fortify_tests.cpp | 7 ++----- tests/fortify_test.cpp | 18 ++++++++++++++++++ 5 files changed, 44 insertions(+), 5 deletions(-) diff --git a/libc/bionic/fortify.cpp b/libc/bionic/fortify.cpp index 20e265e40..cf37666f5 100644 --- a/libc/bionic/fortify.cpp +++ b/libc/bionic/fortify.cpp @@ -500,3 +500,10 @@ extern "C" void* __memcpy_chk(void* dst, const void* src, size_t count, size_t d return memcpy(dst, src, count); } #endif // NO___MEMCPY_CHK + +// Runtime implementation of __mempcpy_chk (used directly by compiler, not in headers). +extern "C" void* __mempcpy_chk(void* dst, const void* src, size_t count, size_t dst_len) { + __check_count("mempcpy", "count", count); + __check_buffer_access("mempcpy", "write into", count, dst_len); + return mempcpy(dst, src, count); +} diff --git a/libc/include/bits/fortify/string.h b/libc/include/bits/fortify/string.h index 0e205d30a..70e4476b4 100644 --- a/libc/include/bits/fortify/string.h +++ b/libc/include/bits/fortify/string.h @@ -66,6 +66,22 @@ void* memmove(void* const dst __pass_object_size0, const void* src, size_t len) } #endif /* __ANDROID_API__ >= __ANDROID_API_J_MR1__ */ +#if defined(__USE_GNU) +#if __ANDROID_API__ >= __ANDROID_API_R__ +__BIONIC_FORTIFY_INLINE +void* mempcpy(void* const dst __pass_object_size0, const void* src, size_t copy_amount) + __overloadable + __clang_error_if(__bos_unevaluated_lt(__bos0(dst), copy_amount), + "'mempcpy' called with size bigger than buffer") { + size_t bos_dst = __bos0(dst); + if (__bos_trivially_not_lt(bos_dst, copy_amount)) { + return __builtin_mempcpy(dst, src, copy_amount); + } + return __builtin___mempcpy_chk(dst, src, copy_amount, bos_dst); +} +#endif /* __ANDROID_API__ >= __ANDROID_API_R__ */ +#endif + #if __ANDROID_API__ >= __ANDROID_API_L__ __BIONIC_FORTIFY_INLINE char* stpcpy(char* const dst __pass_object_size, const char* src) diff --git a/libc/libc.map.txt b/libc/libc.map.txt index b12df56b0..3c23da202 100644 --- a/libc/libc.map.txt +++ b/libc/libc.map.txt @@ -1482,6 +1482,7 @@ LIBC_Q { # introduced=Q LIBC_R { # introduced=R global: + __mempcpy_chk; __open64_2; __openat64_2; call_once; diff --git a/tests/clang_fortify_tests.cpp b/tests/clang_fortify_tests.cpp index 5f8709830..291fc5a2a 100644 --- a/tests/clang_fortify_tests.cpp +++ b/tests/clang_fortify_tests.cpp @@ -159,11 +159,8 @@ FORTIFY_TEST(string) { EXPECT_FORTIFY_DEATH(memcpy(small_buffer, large_buffer, sizeof(large_buffer))); // expected-error@+1{{size bigger than buffer}} EXPECT_FORTIFY_DEATH(memmove(small_buffer, large_buffer, sizeof(large_buffer))); - // FIXME: this should be EXPECT_FORTIFY_DEATH -#if 0 - // expected-error@+1{{called with bigger length than the destination}} -#endif - EXPECT_NO_DEATH(mempcpy(small_buffer, large_buffer, sizeof(large_buffer))); + // expected-error@+1{{size bigger than buffer}} + EXPECT_FORTIFY_DEATH(mempcpy(small_buffer, large_buffer, sizeof(large_buffer))); // expected-error@+1{{size bigger than buffer}} EXPECT_FORTIFY_DEATH(memset(small_buffer, 0, sizeof(large_buffer))); // expected-warning@+1{{arguments got flipped?}} diff --git a/tests/fortify_test.cpp b/tests/fortify_test.cpp index 489b7018d..9a4b781fc 100644 --- a/tests/fortify_test.cpp +++ b/tests/fortify_test.cpp @@ -926,6 +926,24 @@ TEST(TEST_NAME, strcat_chk_max_int_size) { ASSERT_EQ('\0', buf[9]); } +TEST(TEST_NAME, mempcpy_chk) { + const char input_str[] = "abcdefg"; + size_t input_str_size = strlen(input_str) + 1; + + char buf1[10] = {}; + char buf2[10] = {}; + + __builtin_mempcpy(buf1, input_str, input_str_size); + __builtin___mempcpy_chk(buf2, input_str, input_str_size, __bos0(buf2)); + + ASSERT_EQ(memcmp(buf1, buf2, sizeof(buf2)), 0); + + void *builtin_ptr = __builtin_mempcpy(buf1, input_str, input_str_size); + void *fortify_ptr = __builtin___mempcpy_chk(buf1, input_str, input_str_size, __bos0(buf2)); + + ASSERT_EQ(builtin_ptr, fortify_ptr); +} + extern "C" char* __stpcpy_chk(char*, const char*, size_t); TEST(TEST_NAME, stpcpy_chk_max_int_size) { From 261b7f4867f623f3f31609e7574411df43c409e2 Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Mon, 10 Jun 2019 16:32:07 -0700 Subject: [PATCH 3/3] fortify: replace bzero/bcmp defines __builtin_*_chk will emit warnings when things are trivially broken. Emitting errors instead is probably better (and we can be a bit smarter about how we emit code for trivially safe cases.) Bug: 131861088 Test: checkbuild + bionic-unit-tests on blueline Change-Id: I33957ad419922d0760304758ecb9bc8ad33e0b64 --- libc/bionic/ndk_cruft.cpp | 16 ++++++-- libc/include/bits/fortify/strings.h | 59 +++++++++++++++++++++++++++++ libc/include/strings.h | 20 +++++----- tests/clang_fortify_tests.cpp | 5 +-- 4 files changed, 84 insertions(+), 16 deletions(-) create mode 100644 libc/include/bits/fortify/strings.h diff --git a/libc/bionic/ndk_cruft.cpp b/libc/bionic/ndk_cruft.cpp index 2c3299f7b..f0a7026c7 100644 --- a/libc/bionic/ndk_cruft.cpp +++ b/libc/bionic/ndk_cruft.cpp @@ -243,15 +243,23 @@ sighandler_t bsd_signal(int signum, sighandler_t handler) { return signal(signum, handler); } +// bcopy/bzero were previously `#define`d, so we only have `static` wrappers in +// Bionic headers. Since we have header definitions, we need some way to +// overload these implementations; __never_call will ensure that any calls to +// bcopy/bzero call the in-header implementation. Since the implementations +// should end up functionally identical, it doesn't matter which we actually +// call. +#define __never_call __attribute__((enable_if(false, "never selected"))) + // This was removed from POSIX 2008. -#undef bcopy -void bcopy(const void* src, void* dst, size_t n) { +void bcopy(const void* src, void* dst, size_t n) __never_call __RENAME(bcopy); +void bcopy(const void* src, void* dst, size_t n) __never_call { memmove(dst, src, n); } // This was removed from POSIX 2008. -#undef bzero -void bzero(void* dst, size_t n) { +void bzero(void* dst, size_t n) __never_call __RENAME(bzero); +void bzero(void* dst, size_t n) __never_call { memset(dst, 0, n); } diff --git a/libc/include/bits/fortify/strings.h b/libc/include/bits/fortify/strings.h new file mode 100644 index 000000000..6bda295f3 --- /dev/null +++ b/libc/include/bits/fortify/strings.h @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#if defined(__BIONIC_FORTIFY) + +#if __ANDROID_API__ >= __ANDROID_API_J_MR1__ +__BIONIC_FORTIFY_INLINE +void bcopy(const void *src, void* const dst __pass_object_size0, size_t len) + __overloadable + __clang_error_if(__bos_unevaluated_lt(__bos0(dst), len), + "'bcopy' called with size bigger than buffer") { + size_t bos = __bos0(dst); + if (__bos_trivially_not_lt(bos, len)) { + __builtin_memmove(dst, src, len); + } else { + __builtin___memmove_chk(dst, src, len, bos); + } +} + +__BIONIC_FORTIFY_INLINE +void bzero(void* const b __pass_object_size0, size_t len) + __overloadable + __clang_error_if(__bos_unevaluated_lt(__bos0(b), len), + "'bzero' called with size bigger than buffer") { + size_t bos = __bos0(b); + if (__bos_trivially_not_lt(bos, len)) { + __builtin_memset(b, 0, len); + } else { + __builtin___memset_chk(b, 0, len, bos); + } +} +#endif /* __ANDROID_API__ >= __ANDROID_API_J_MR1__ */ + +#endif /* defined(__BIONIC_FORTIFY) */ diff --git a/libc/include/strings.h b/libc/include/strings.h index ccdac044c..a054aed55 100644 --- a/libc/include/strings.h +++ b/libc/include/strings.h @@ -51,17 +51,15 @@ __BEGIN_DECLS -#if defined(__BIONIC_FORTIFY) /** Deprecated. Use memmove() instead. */ -#define bcopy(b1, b2, len) (void)(__builtin___memmove_chk((b2), (b1), (len), __bos0(b2))) +static __inline__ __always_inline void bcopy(const void* b1, void* b2, size_t len) { + __builtin_memmove(b2, b1, len); +} + /** Deprecated. Use memset() instead. */ -#define bzero(b, len) (void)(__builtin___memset_chk((b), '\0', (len), __bos0(b))) -#else -/** Deprecated. Use memmove() instead. */ -#define bcopy(b1, b2, len) (void)(__builtin_memmove((b2), (b1), (len))) -/** Deprecated. Use memset() instead. */ -#define bzero(b, len) (void)(__builtin_memset((b), '\0', (len))) -#endif +static __inline__ __always_inline void bzero(void* b, size_t len) { + __builtin_memset(b, 0, len); +} #if !defined(__i386__) || __ANDROID_API__ >= __ANDROID_API_J_MR2__ /** @@ -72,6 +70,10 @@ __BEGIN_DECLS int ffs(int __i) __INTRODUCED_IN_X86(18); #endif +#if defined(__BIONIC_INCLUDE_FORTIFY_HEADERS) +#include +#endif + __END_DECLS #include diff --git a/tests/clang_fortify_tests.cpp b/tests/clang_fortify_tests.cpp index 291fc5a2a..1b6b898d5 100644 --- a/tests/clang_fortify_tests.cpp +++ b/tests/clang_fortify_tests.cpp @@ -165,10 +165,9 @@ FORTIFY_TEST(string) { EXPECT_FORTIFY_DEATH(memset(small_buffer, 0, sizeof(large_buffer))); // expected-warning@+1{{arguments got flipped?}} EXPECT_NO_DEATH(memset(small_buffer, sizeof(small_buffer), 0)); - // FIXME: Should these be warnings? - // expected-warning@+1{{will always overflow}} + // expected-error@+1{{size bigger than buffer}} EXPECT_FORTIFY_DEATH(bcopy(large_buffer, small_buffer, sizeof(large_buffer))); - // expected-warning@+1{{will always overflow}} + // expected-error@+1{{size bigger than buffer}} EXPECT_FORTIFY_DEATH(bzero(small_buffer, sizeof(large_buffer))); }