From: Nick Desaulniers <ndesaulniers@google.com>
To: Kees Cook <keescook@chromium.org>
Cc: Nathan Chancellor <nathan@kernel.org>, Tom Rix <trix@redhat.com>,
linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
llvm@lists.linux.dev, Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
linux-input@vger.kernel.org,
Masahiro Yamada <masahiroy@kernel.org>,
Nick Desaulniers <ndesaulniers@google.com>
Subject: [PATCH 1/3] fortify: use __builtin_dynamic_object_size in __compiletime_strlen
Date: Tue, 30 Aug 2022 13:53:07 -0700 [thread overview]
Message-ID: <20220830205309.312864-2-ndesaulniers@google.com> (raw)
In-Reply-To: <20220830205309.312864-1-ndesaulniers@google.com>
With CONFIG_FORTIFY=y and CONFIG_UBSAN_LOCAL_BOUNDS=y enabled, we
observe a runtime panic while running Android's Compatibility Test
Suite's (CTS) android.hardware.input.cts.tests. This is stemming from a
strlen() call in hidinput_allocate().
__compiletime_strlen is implemented in terms of __builtin_object_size(),
then does an array access to check for NUL-termination. A quirk of
__builtin_object_size() is that for strings whose values are runtime
dependent, __builtin_object_size(str, 1 or 0) returns the maximum size
of possible values when those sizes are determinable at compile time.
Example:
static const char *v = "FOO BAR";
static const char *y = "FOO BA";
unsigned long x (int z) {
// Returns 8, which is:
// max(__builtin_object_size(v, 1), __builtin_object_size(y, 1))
return __builtin_object_size(z ? v : y, 1);
}
So when FORTIFY is enabled, the current implementation of
__compiletime_strlen will try to access beyond the end of y at runtime
using the size of v. Mixed with UBSAN_LOCAL_BOUNDS we get a fault.
hidinput_allocate() has a local C string whose value is control flow
dependent on a switch statement, so __builtin_object_size(str, 1)
evaluates to the maximum string length, making all other cases fault on
the last character check. hidinput_allocate() could be cleaned up to
avoid runtime calls to strlen() since the local variable can only have
literal values, so there's no benefit to trying to fortify the strlen
call site there.
Add a Kconfig check for __builtin_dynamic_object_size(), then use that
when available (gcc-12+, all supported versions of clang) which avoids
this surprising behavior.
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
include/linux/fortify-string.h | 8 +++++++-
init/Kconfig | 3 +++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 3b401fa0f374..c5adad596a3f 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -14,11 +14,17 @@ void __read_overflow2_field(size_t avail, size_t wanted) __compiletime_warning("
void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?");
+#ifdef CONFIG_CC_HAS_BUILTIN_DYNAMIC_OBJECT_SIZE
+#define __object_size __builtin_dynamic_object_size
+#else
+#define __object_size __builtin_object_size
+#endif
+
#define __compiletime_strlen(p) \
({ \
unsigned char *__p = (unsigned char *)(p); \
size_t __ret = (size_t)-1; \
- size_t __p_size = __builtin_object_size(p, 1); \
+ size_t __p_size = __object_size(p, 1); \
if (__p_size != (size_t)-1) { \
size_t __p_len = __p_size - 1; \
if (__builtin_constant_p(__p[__p_len]) && \
diff --git a/init/Kconfig b/init/Kconfig
index 532362fcfe31..87dd31aa54ad 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -876,6 +876,9 @@ config ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
config CC_HAS_INT128
def_bool !$(cc-option,$(m64-flag) -D__SIZEOF_INT128__=0) && 64BIT
+config CC_HAS_BUILTIN_DYNAMIC_OBJECT_SIZE
+ def_bool !CC_IS_GCC || GCC_VERSION >= 120000
+
config CC_IMPLICIT_FALLTHROUGH
string
default "-Wimplicit-fallthrough=5" if CC_IS_GCC && $(cc-option,-Wimplicit-fallthrough=5)
--
2.37.2.672.g94769d06f0-goog
next prev parent reply other threads:[~2022-08-30 20:53 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-30 20:53 [PATCH 0/3] Fix FORTIFY=y UBSAN_LOCAL_BOUNDS=y Nick Desaulniers
2022-08-30 20:53 ` Nick Desaulniers [this message]
2022-08-31 18:34 ` [PATCH 1/3] fortify: use __builtin_dynamic_object_size in __compiletime_strlen Kees Cook
2022-08-30 20:53 ` [PATCH 2/3] fortify: cosmetic cleanups to __compiletime_strlen Nick Desaulniers
2022-08-31 13:13 ` kernel test robot
2022-08-31 19:06 ` Kees Cook
2022-08-30 20:53 ` [PATCH 3/3] HID: avoid runtime call to strlen Nick Desaulniers
2022-08-31 6:05 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220830205309.312864-2-ndesaulniers@google.com \
--to=ndesaulniers@google.com \
--cc=benjamin.tissoires@redhat.com \
--cc=jikos@kernel.org \
--cc=keescook@chromium.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=masahiroy@kernel.org \
--cc=nathan@kernel.org \
--cc=trix@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox