* [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available
@ 2022-09-20 19:21 Kees Cook
2022-09-20 19:21 ` [PATCH 1/4] x86/entry: Work around Clang __bdos() bug Kees Cook
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Kees Cook @ 2022-09-20 19:21 UTC (permalink / raw)
To: linux-hardening
Cc: Kees Cook, Siddhesh Poyarekar, Nathan Chancellor,
Nick Desaulniers, Arnd Bergmann, Juergen Gross, Boris Ostrovsky,
Tom Rix, Miguel Ojeda, linux-kernel, llvm
Hi,
This adjusts CONFIG_FORTIFY_SOURCE's coverage to include greater runtime
size checking from GCC and Clang's __builtin_dynamic_object_size(), which
the compilers can track either via code flow or from __alloc_size() hints.
Thanks,
-Kees
Kees Cook (4):
x86/entry: Work around Clang __bdos() bug
fortify: Explicitly check bounds are compile-time constants
fortify: Convert to struct vs member helpers
fortify: Use __builtin_dynamic_object_size() when available
arch/x86/xen/enlighten_pv.c | 3 +-
drivers/misc/lkdtm/heap.c | 1 +
include/linux/compiler_attributes.h | 5 ++
include/linux/fortify-string.h | 125 ++++++++++++++++------------
4 files changed, 81 insertions(+), 53 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/4] x86/entry: Work around Clang __bdos() bug
2022-09-20 19:21 [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
@ 2022-09-20 19:21 ` Kees Cook
2022-09-21 0:07 ` Boris Ostrovsky
2022-09-20 19:22 ` [PATCH 2/4] fortify: Explicitly check bounds are compile-time constants Kees Cook
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2022-09-20 19:21 UTC (permalink / raw)
To: linux-hardening
Cc: Kees Cook, Juergen Gross, Boris Ostrovsky, Nathan Chancellor,
Nick Desaulniers, xen-devel, llvm, Siddhesh Poyarekar,
Arnd Bergmann, Tom Rix, Miguel Ojeda, linux-kernel
After expanding bounds checking to use __builtin_dynamic_object_size(),
Clang produces a false positive when building with CONFIG_FORTIFY_SOURCE=y
and CONFIG_UBSAN_BOUNDS=y when operating on an array with a dynamic
offset. Work around this by using a direct assignment of an empty
instance. Avoids this warning:
../include/linux/fortify-string.h:309:4: warning: call to __write_overflow_field declared with 'warn
ing' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wat
tribute-warning]
__write_overflow_field(p_size_field, size);
^
which was isolated to the memset() call in xen_load_idt().
Note that this looks very much like another bug that was worked around:
https://github.com/ClangBuiltLinux/linux/issues/1592
Cc: Juergen Gross <jgross@suse.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: xen-devel@lists.xenproject.org
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
arch/x86/xen/enlighten_pv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 0ed2e487a693..9b1a58dda935 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -765,6 +765,7 @@ static void xen_load_idt(const struct desc_ptr *desc)
{
static DEFINE_SPINLOCK(lock);
static struct trap_info traps[257];
+ static const struct trap_info zero = { };
unsigned out;
trace_xen_cpu_load_idt(desc);
@@ -774,7 +775,7 @@ static void xen_load_idt(const struct desc_ptr *desc)
memcpy(this_cpu_ptr(&idt_desc), desc, sizeof(idt_desc));
out = xen_convert_trap_info(desc, traps, false);
- memset(&traps[out], 0, sizeof(traps[0]));
+ traps[out] = zero;
xen_mc_flush();
if (HYPERVISOR_set_trap_table(traps))
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/4] fortify: Explicitly check bounds are compile-time constants
2022-09-20 19:21 [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
2022-09-20 19:21 ` [PATCH 1/4] x86/entry: Work around Clang __bdos() bug Kees Cook
@ 2022-09-20 19:22 ` Kees Cook
2022-09-21 11:48 ` Siddhesh Poyarekar
2022-09-20 19:22 ` [PATCH 3/4] fortify: Convert to struct vs member helpers Kees Cook
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2022-09-20 19:22 UTC (permalink / raw)
To: linux-hardening
Cc: Kees Cook, Siddhesh Poyarekar, Nathan Chancellor,
Nick Desaulniers, Arnd Bergmann, Juergen Gross, Boris Ostrovsky,
Tom Rix, Miguel Ojeda, linux-kernel, llvm
In preparation for replacing __builtin_object_size() with
__builtin_dynamic_object_size(), all the compile-time size checks need
to check that the bounds variables are, in fact, known at compile-time.
Enforce what was guaranteed with __bos(). In other words, since all uses
of __bos() were constant expressions, it was not required to test for
this. When these change to __bdos(), they _may_ be constant expressions,
and the checks are only valid when the prior condition holds. This
results in no binary differences.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/fortify-string.h | 50 +++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index ff879efe94ed..71c0a432c638 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -80,6 +80,12 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
#define POS __pass_object_size(1)
#define POS0 __pass_object_size(0)
+#define __compiletime_lessthan(bounds, length) ( \
+ __builtin_constant_p(length) && \
+ __builtin_constant_p(bounds) && \
+ bounds < length \
+)
+
/**
* strncpy - Copy a string to memory with non-guaranteed NUL padding
*
@@ -117,7 +123,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
{
size_t p_size = __builtin_object_size(p, 1);
- if (__builtin_constant_p(size) && p_size < size)
+ if (__compiletime_lessthan(p_size, size))
__write_overflow();
if (p_size < size)
fortify_panic(__func__);
@@ -224,7 +230,7 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
* If size can be known at compile time and is greater than
* p_size, generate a compile time write overflow error.
*/
- if (__builtin_constant_p(size) && size > p_size)
+ if (__compiletime_lessthan(p_size, size))
__write_overflow();
/*
@@ -281,15 +287,16 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
/*
* Length argument is a constant expression, so we
* can perform compile-time bounds checking where
- * buffer sizes are known.
+ * buffer sizes are also known at compile time.
*/
/* Error when size is larger than enclosing struct. */
- if (p_size > p_size_field && p_size < size)
+ if (__compiletime_lessthan(p_size_field, p_size) &&
+ __compiletime_lessthan(p_size, size))
__write_overflow();
/* Warn when write size is larger than dest field. */
- if (p_size_field < size)
+ if (__compiletime_lessthan(p_size_field, size))
__write_overflow_field(p_size_field, size);
}
/*
@@ -365,25 +372,28 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
/*
* Length argument is a constant expression, so we
* can perform compile-time bounds checking where
- * buffer sizes are known.
+ * buffer sizes are also known at compile time.
*/
/* Error when size is larger than enclosing struct. */
- if (p_size > p_size_field && p_size < size)
+ if (__compiletime_lessthan(p_size_field, p_size) &&
+ __compiletime_lessthan(p_size, size))
__write_overflow();
- if (q_size > q_size_field && q_size < size)
+ if (__compiletime_lessthan(q_size_field, q_size) &&
+ __compiletime_lessthan(q_size, size))
__read_overflow2();
/* Warn when write size argument larger than dest field. */
- if (p_size_field < size)
+ if (__compiletime_lessthan(p_size_field, size))
__write_overflow_field(p_size_field, size);
/*
* Warn for source field over-read when building with W=1
* or when an over-write happened, so both can be fixed at
* the same time.
*/
- if ((IS_ENABLED(KBUILD_EXTRA_WARN1) || p_size_field < size) &&
- q_size_field < size)
+ if ((IS_ENABLED(KBUILD_EXTRA_WARN1) ||
+ __compiletime_lessthan(p_size_field, size)) &&
+ __compiletime_lessthan(q_size_field, size))
__read_overflow2_field(q_size_field, size);
}
/*
@@ -494,7 +504,7 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
{
size_t p_size = __builtin_object_size(p, 0);
- if (__builtin_constant_p(size) && p_size < size)
+ if (__compiletime_lessthan(p_size, size))
__read_overflow();
if (p_size < size)
fortify_panic(__func__);
@@ -508,9 +518,9 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t
size_t q_size = __builtin_object_size(q, 0);
if (__builtin_constant_p(size)) {
- if (p_size < size)
+ if (__compiletime_lessthan(p_size, size))
__read_overflow();
- if (q_size < size)
+ if (__compiletime_lessthan(q_size, size))
__read_overflow2();
}
if (p_size < size || q_size < size)
@@ -523,7 +533,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
{
size_t p_size = __builtin_object_size(p, 0);
- if (__builtin_constant_p(size) && p_size < size)
+ if (__compiletime_lessthan(p_size, size))
__read_overflow();
if (p_size < size)
fortify_panic(__func__);
@@ -535,7 +545,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
{
size_t p_size = __builtin_object_size(p, 0);
- if (__builtin_constant_p(size) && p_size < size)
+ if (__compiletime_lessthan(p_size, size))
__read_overflow();
if (p_size < size)
fortify_panic(__func__);
@@ -547,7 +557,7 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp
{
size_t p_size = __builtin_object_size(p, 0);
- if (__builtin_constant_p(size) && p_size < size)
+ if (__compiletime_lessthan(p_size, size))
__read_overflow();
if (p_size < size)
fortify_panic(__func__);
@@ -563,11 +573,13 @@ char *strcpy(char * const POS p, const char * const POS q)
size_t size;
/* If neither buffer size is known, immediately give up. */
- if (p_size == SIZE_MAX && q_size == SIZE_MAX)
+ if (__builtin_constant_p(p_size) &&
+ __builtin_constant_p(q_size) &&
+ p_size == SIZE_MAX && q_size == SIZE_MAX)
return __underlying_strcpy(p, q);
size = strlen(q) + 1;
/* Compile-time check for const size overflow. */
- if (__builtin_constant_p(size) && p_size < size)
+ if (__compiletime_lessthan(p_size, size))
__write_overflow();
/* Run-time check for dynamic size overflow. */
if (p_size < size)
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/4] fortify: Convert to struct vs member helpers
2022-09-20 19:21 [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
2022-09-20 19:21 ` [PATCH 1/4] x86/entry: Work around Clang __bdos() bug Kees Cook
2022-09-20 19:22 ` [PATCH 2/4] fortify: Explicitly check bounds are compile-time constants Kees Cook
@ 2022-09-20 19:22 ` Kees Cook
2022-09-20 19:22 ` [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
2022-09-22 20:26 ` [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Siddhesh Poyarekar
4 siblings, 0 replies; 22+ messages in thread
From: Kees Cook @ 2022-09-20 19:22 UTC (permalink / raw)
To: linux-hardening
Cc: Kees Cook, Nathan Chancellor, Nick Desaulniers, Tom Rix, llvm,
Siddhesh Poyarekar, Arnd Bergmann, Juergen Gross,
Boris Ostrovsky, Miguel Ojeda, linux-kernel
In preparation for adding support for __builtin_dynamic_object_size(),
wrap each instance of __builtin_object_size(p, N) with either the new
__struct_size(p) as __bos(p, 0), or __member_size(p) as __bos(p, 1).
This will allow us to replace the definitions with __bdos() next.
There are no binary differences from this change.
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Tom Rix <trix@redhat.com>
Cc: linux-hardening@vger.kernel.org
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/fortify-string.h | 68 +++++++++++++++++-----------------
1 file changed, 35 insertions(+), 33 deletions(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 71c0a432c638..3f1178584d7b 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -20,7 +20,7 @@ void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning("
({ \
unsigned char *__p = (unsigned char *)(p); \
size_t __ret = SIZE_MAX; \
- size_t __p_size = __builtin_object_size(p, 1); \
+ size_t __p_size = __member_size(p); \
if (__p_size != SIZE_MAX && \
__builtin_constant_p(*__p)) { \
size_t __p_len = __p_size - 1; \
@@ -72,13 +72,15 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
__underlying_memcpy(dst, src, bytes)
/*
- * Clang's use of __builtin_object_size() within inlines needs hinting via
- * __pass_object_size(). The preference is to only ever use type 1 (member
+ * Clang's use of __builtin_*object_size() within inlines needs hinting via
+ * __pass_*object_size(). The preference is to only ever use type 1 (member
* size, rather than struct size), but there remain some stragglers using
* type 0 that will be converted in the future.
*/
-#define POS __pass_object_size(1)
-#define POS0 __pass_object_size(0)
+#define POS __pass_object_size(1)
+#define POS0 __pass_object_size(0)
+#define __struct_size(p) __builtin_object_size(p, 0)
+#define __member_size(p) __builtin_object_size(p, 1)
#define __compiletime_lessthan(bounds, length) ( \
__builtin_constant_p(length) && \
@@ -121,7 +123,7 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
__FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3)
char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
{
- size_t p_size = __builtin_object_size(p, 1);
+ size_t p_size = __member_size(p);
if (__compiletime_lessthan(p_size, size))
__write_overflow();
@@ -133,7 +135,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
__FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
char *strcat(char * const POS p, const char *q)
{
- size_t p_size = __builtin_object_size(p, 1);
+ size_t p_size = __member_size(p);
if (p_size == SIZE_MAX)
return __underlying_strcat(p, q);
@@ -145,7 +147,7 @@ char *strcat(char * const POS p, const char *q)
extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
__FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size_t maxlen)
{
- size_t p_size = __builtin_object_size(p, 1);
+ size_t p_size = __member_size(p);
size_t p_len = __compiletime_strlen(p);
size_t ret;
@@ -175,7 +177,7 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1)
__kernel_size_t __fortify_strlen(const char * const POS p)
{
__kernel_size_t ret;
- size_t p_size = __builtin_object_size(p, 1);
+ size_t p_size = __member_size(p);
/* Give up if we don't know how large p is. */
if (p_size == SIZE_MAX)
@@ -190,8 +192,8 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
__FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, size_t size)
{
- size_t p_size = __builtin_object_size(p, 1);
- size_t q_size = __builtin_object_size(q, 1);
+ size_t p_size = __member_size(p);
+ size_t q_size = __member_size(q);
size_t q_len; /* Full count of source string length. */
size_t len; /* Count of characters going into destination. */
@@ -219,8 +221,8 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
{
size_t len;
/* Use string size rather than possible enclosing struct size. */
- size_t p_size = __builtin_object_size(p, 1);
- size_t q_size = __builtin_object_size(q, 1);
+ size_t p_size = __member_size(p);
+ size_t q_size = __member_size(q);
/* If we cannot get size of p and q default to call strscpy. */
if (p_size == SIZE_MAX && q_size == SIZE_MAX)
@@ -265,8 +267,8 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strncat, 1, 2, 3)
char *strncat(char * const POS p, const char * const POS q, __kernel_size_t count)
{
size_t p_len, copy_len;
- size_t p_size = __builtin_object_size(p, 1);
- size_t q_size = __builtin_object_size(q, 1);
+ size_t p_size = __member_size(p);
+ size_t q_size = __member_size(q);
if (p_size == SIZE_MAX && q_size == SIZE_MAX)
return __underlying_strncat(p, q, count);
@@ -324,11 +326,11 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
})
/*
- * __builtin_object_size() must be captured here to avoid evaluating argument
- * side-effects further into the macro layers.
+ * __struct_size() vs __member_size() must be captured here to avoid
+ * evaluating argument side-effects further into the macro layers.
*/
#define memset(p, c, s) __fortify_memset_chk(p, c, s, \
- __builtin_object_size(p, 0), __builtin_object_size(p, 1))
+ __struct_size(p), __member_size(p))
/*
* To make sure the compiler can enforce protection against buffer overflows,
@@ -421,7 +423,7 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
* fake flexible arrays, until they are all converted to
* proper flexible arrays.
*
- * The implementation of __builtin_object_size() behaves
+ * The implementation of __builtin_*object_size() behaves
* like sizeof() when not directly referencing a flexible
* array member, which means there will be many bounds checks
* that will appear at run-time, without a way for them to be
@@ -487,22 +489,22 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
*/
/*
- * __builtin_object_size() must be captured here to avoid evaluating argument
- * side-effects further into the macro layers.
+ * __struct_size() vs __member_size() must be captured here to avoid
+ * evaluating argument side-effects further into the macro layers.
*/
#define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
- __builtin_object_size(p, 0), __builtin_object_size(q, 0), \
- __builtin_object_size(p, 1), __builtin_object_size(q, 1), \
+ __struct_size(p), __struct_size(q), \
+ __member_size(p), __member_size(q), \
memcpy)
#define memmove(p, q, s) __fortify_memcpy_chk(p, q, s, \
- __builtin_object_size(p, 0), __builtin_object_size(q, 0), \
- __builtin_object_size(p, 1), __builtin_object_size(q, 1), \
+ __struct_size(p), __struct_size(q), \
+ __member_size(p), __member_size(q), \
memmove)
extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan);
__FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
{
- size_t p_size = __builtin_object_size(p, 0);
+ size_t p_size = __struct_size(p);
if (__compiletime_lessthan(p_size, size))
__read_overflow();
@@ -514,8 +516,8 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
__FORTIFY_INLINE __diagnose_as(__builtin_memcmp, 1, 2, 3)
int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t size)
{
- size_t p_size = __builtin_object_size(p, 0);
- size_t q_size = __builtin_object_size(q, 0);
+ size_t p_size = __struct_size(p);
+ size_t q_size = __struct_size(q);
if (__builtin_constant_p(size)) {
if (__compiletime_lessthan(p_size, size))
@@ -531,7 +533,7 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t
__FORTIFY_INLINE __diagnose_as(__builtin_memchr, 1, 2, 3)
void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
{
- size_t p_size = __builtin_object_size(p, 0);
+ size_t p_size = __struct_size(p);
if (__compiletime_lessthan(p_size, size))
__read_overflow();
@@ -543,7 +545,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
void *__real_memchr_inv(const void *s, int c, size_t n) __RENAME(memchr_inv);
__FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
{
- size_t p_size = __builtin_object_size(p, 0);
+ size_t p_size = __struct_size(p);
if (__compiletime_lessthan(p_size, size))
__read_overflow();
@@ -555,7 +557,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
extern void *__real_kmemdup(const void *src, size_t len, gfp_t gfp) __RENAME(kmemdup);
__FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp)
{
- size_t p_size = __builtin_object_size(p, 0);
+ size_t p_size = __struct_size(p);
if (__compiletime_lessthan(p_size, size))
__read_overflow();
@@ -568,8 +570,8 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp
__FORTIFY_INLINE __diagnose_as(__builtin_strcpy, 1, 2)
char *strcpy(char * const POS p, const char * const POS q)
{
- size_t p_size = __builtin_object_size(p, 1);
- size_t q_size = __builtin_object_size(q, 1);
+ size_t p_size = __member_size(p);
+ size_t q_size = __member_size(q);
size_t size;
/* If neither buffer size is known, immediately give up. */
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available
2022-09-20 19:21 [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
` (2 preceding siblings ...)
2022-09-20 19:22 ` [PATCH 3/4] fortify: Convert to struct vs member helpers Kees Cook
@ 2022-09-20 19:22 ` Kees Cook
2022-09-21 11:24 ` Miguel Ojeda
` (3 more replies)
2022-09-22 20:26 ` [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Siddhesh Poyarekar
4 siblings, 4 replies; 22+ messages in thread
From: Kees Cook @ 2022-09-20 19:22 UTC (permalink / raw)
To: linux-hardening
Cc: Kees Cook, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann,
Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm,
Juergen Gross, Boris Ostrovsky, linux-kernel
Since the commits starting with c37495d6254c ("slab: add __alloc_size
attributes for better bounds checking"), the compilers have runtime
allocation size hints available in some places. This was immediately
available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
updating to explicitly make use the hints via the associated
__builtin_dynamic_object_size() helper. Detect and use the builtin when
it is available, increasing the accuracy of the mitigation. When runtime
sizes are not available, __builtin_dynamic_object_size() falls back to
__builtin_object_size(), leaving the existing bounds checking unchanged.
Additionally update the VMALLOC_LINEAR_OVERFLOW LKDTM test to make the
hint invisible, otherwise the architectural defense is not exercised
(the buffer overflow is detected in the memset() rather than when it
crosses the edge of the allocation).
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Siddhesh Poyarekar <siddhesh@gotplt.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Tom Rix <trix@redhat.com>
Cc: linux-hardening@vger.kernel.org
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/misc/lkdtm/heap.c | 1 +
include/linux/compiler_attributes.h | 5 +++++
include/linux/fortify-string.h | 7 +++++++
3 files changed, 13 insertions(+)
diff --git a/drivers/misc/lkdtm/heap.c b/drivers/misc/lkdtm/heap.c
index 62516078a619..0ce4cbf6abda 100644
--- a/drivers/misc/lkdtm/heap.c
+++ b/drivers/misc/lkdtm/heap.c
@@ -31,6 +31,7 @@ static void lkdtm_VMALLOC_LINEAR_OVERFLOW(void)
char *one, *two;
one = vzalloc(PAGE_SIZE);
+ OPTIMIZER_HIDE_VAR(one);
two = vzalloc(PAGE_SIZE);
pr_info("Attempting vmalloc linear overflow ...\n");
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 445e80517cab..9a9907fad6fd 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -296,6 +296,11 @@
*
* clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size
*/
+#if __has_attribute(__pass_dynamic_object_size__)
+# define __pass_dynamic_object_size(type) __attribute__((__pass_dynamic_object_size__(type)))
+#else
+# define __pass_dynamic_object_size(type)
+#endif
#if __has_attribute(__pass_object_size__)
# define __pass_object_size(type) __attribute__((__pass_object_size__(type)))
#else
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 3f1178584d7b..dd7f85d74ade 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -77,10 +77,17 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
* size, rather than struct size), but there remain some stragglers using
* type 0 that will be converted in the future.
*/
+#if __has_builtin(__builtin_dynamic_object_size)
+#define POS __pass_dynamic_object_size(1)
+#define POS0 __pass_dynamic_object_size(0)
+#define __struct_size(p) __builtin_dynamic_object_size(p, 0)
+#define __member_size(p) __builtin_dynamic_object_size(p, 1)
+#else
#define POS __pass_object_size(1)
#define POS0 __pass_object_size(0)
#define __struct_size(p) __builtin_object_size(p, 0)
#define __member_size(p) __builtin_object_size(p, 1)
+#endif
#define __compiletime_lessthan(bounds, length) ( \
__builtin_constant_p(length) && \
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/4] x86/entry: Work around Clang __bdos() bug
2022-09-20 19:21 ` [PATCH 1/4] x86/entry: Work around Clang __bdos() bug Kees Cook
@ 2022-09-21 0:07 ` Boris Ostrovsky
0 siblings, 0 replies; 22+ messages in thread
From: Boris Ostrovsky @ 2022-09-21 0:07 UTC (permalink / raw)
To: Kees Cook, linux-hardening
Cc: Juergen Gross, Nathan Chancellor, Nick Desaulniers, xen-devel,
llvm, Siddhesh Poyarekar, Arnd Bergmann, Tom Rix, Miguel Ojeda,
linux-kernel
On 9/20/22 3:21 PM, Kees Cook wrote:
> After expanding bounds checking to use __builtin_dynamic_object_size(),
> Clang produces a false positive when building with CONFIG_FORTIFY_SOURCE=y
> and CONFIG_UBSAN_BOUNDS=y when operating on an array with a dynamic
> offset. Work around this by using a direct assignment of an empty
> instance. Avoids this warning:
>
> ../include/linux/fortify-string.h:309:4: warning: call to __write_overflow_field declared with 'warn
> ing' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wat
> tribute-warning]
> __write_overflow_field(p_size_field, size);
> ^
>
> which was isolated to the memset() call in xen_load_idt().
>
> Note that this looks very much like another bug that was worked around:
> https://github.com/ClangBuiltLinux/linux/issues/1592
>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: xen-devel@lists.xenproject.org
> Cc: llvm@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available
2022-09-20 19:22 ` [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
@ 2022-09-21 11:24 ` Miguel Ojeda
2022-09-21 11:43 ` Siddhesh Poyarekar
` (2 subsequent siblings)
3 siblings, 0 replies; 22+ messages in thread
From: Miguel Ojeda @ 2022-09-21 11:24 UTC (permalink / raw)
To: Kees Cook
Cc: linux-hardening, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann,
Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm,
Juergen Gross, Boris Ostrovsky, linux-kernel
On Tue, Sep 20, 2022 at 9:22 PM Kees Cook <keescook@chromium.org> wrote:
>
> include/linux/compiler_attributes.h | 5 +++++
Reviewed-by: Miguel Ojeda <ojeda@kernel.org>
Cheers,
Miguel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available
2022-09-20 19:22 ` [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
2022-09-21 11:24 ` Miguel Ojeda
@ 2022-09-21 11:43 ` Siddhesh Poyarekar
2022-09-22 3:33 ` Kees Cook
2022-11-22 10:20 ` Siddhesh Poyarekar
2023-01-13 15:59 ` linux-next - bxnt buffer overflow in strnlen Niklas Cassel
3 siblings, 1 reply; 22+ messages in thread
From: Siddhesh Poyarekar @ 2022-09-21 11:43 UTC (permalink / raw)
To: Kees Cook, linux-hardening
Cc: Miguel Ojeda, Arnd Bergmann, Nick Desaulniers, Nathan Chancellor,
Tom Rix, llvm, Juergen Gross, Boris Ostrovsky, linux-kernel
On 2022-09-20 15:22, Kees Cook wrote:
> Since the commits starting with c37495d6254c ("slab: add __alloc_size
> attributes for better bounds checking"), the compilers have runtime
> allocation size hints available in some places. This was immediately
> available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
> updating to explicitly make use the hints via the associated
> __builtin_dynamic_object_size() helper. Detect and use the builtin when
> it is available, increasing the accuracy of the mitigation. When runtime
> sizes are not available, __builtin_dynamic_object_size() falls back to
> __builtin_object_size(), leaving the existing bounds checking unchanged.
I don't know yet what the overhead is for __builtin_dynamic_object_size
vs __builtin_object_size, were you able to measure it somehow for the
kernel? If there's a significant tradeoff, it may make sense to provide
a user override.
Thanks,
Sid
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/4] fortify: Explicitly check bounds are compile-time constants
2022-09-20 19:22 ` [PATCH 2/4] fortify: Explicitly check bounds are compile-time constants Kees Cook
@ 2022-09-21 11:48 ` Siddhesh Poyarekar
2022-09-22 3:46 ` Kees Cook
0 siblings, 1 reply; 22+ messages in thread
From: Siddhesh Poyarekar @ 2022-09-21 11:48 UTC (permalink / raw)
To: Kees Cook, linux-hardening
Cc: Nathan Chancellor, Nick Desaulniers, Arnd Bergmann,
Juergen Gross, Boris Ostrovsky, Tom Rix, Miguel Ojeda,
linux-kernel, llvm
On 2022-09-20 15:22, Kees Cook wrote:
> In preparation for replacing __builtin_object_size() with
> __builtin_dynamic_object_size(), all the compile-time size checks need
> to check that the bounds variables are, in fact, known at compile-time.
> Enforce what was guaranteed with __bos(). In other words, since all uses
> of __bos() were constant expressions, it was not required to test for
> this. When these change to __bdos(), they _may_ be constant expressions,
> and the checks are only valid when the prior condition holds. This
> results in no binary differences.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> include/linux/fortify-string.h | 50 +++++++++++++++++++++-------------
> 1 file changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index ff879efe94ed..71c0a432c638 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -80,6 +80,12 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
> #define POS __pass_object_size(1)
> #define POS0 __pass_object_size(0)
>
> +#define __compiletime_lessthan(bounds, length) ( \
> + __builtin_constant_p(length) && \
> + __builtin_constant_p(bounds) && \
> + bounds < length \
> +)
So with the gcc ranger, the compiler has lately been quite successful at
computing a constant `bounds < length` even though bounds and length are
not constant. So perhaps this:
#define __compiletime_lessthan (bounds, length) ( \
__builtin_constant (bounds < length) && \
bounds < length \
)
might succeed in a few more cases.
Thanks,
Sid
> +
> /**
> * strncpy - Copy a string to memory with non-guaranteed NUL padding
> *
> @@ -117,7 +123,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
> {
> size_t p_size = __builtin_object_size(p, 1);
>
> - if (__builtin_constant_p(size) && p_size < size)
> + if (__compiletime_lessthan(p_size, size))
> __write_overflow();
> if (p_size < size)
> fortify_panic(__func__);
> @@ -224,7 +230,7 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
> * If size can be known at compile time and is greater than
> * p_size, generate a compile time write overflow error.
> */
> - if (__builtin_constant_p(size) && size > p_size)
> + if (__compiletime_lessthan(p_size, size))
> __write_overflow();
>
> /*
> @@ -281,15 +287,16 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
> /*
> * Length argument is a constant expression, so we
> * can perform compile-time bounds checking where
> - * buffer sizes are known.
> + * buffer sizes are also known at compile time.
> */
>
> /* Error when size is larger than enclosing struct. */
> - if (p_size > p_size_field && p_size < size)
> + if (__compiletime_lessthan(p_size_field, p_size) &&
> + __compiletime_lessthan(p_size, size))
> __write_overflow();
>
> /* Warn when write size is larger than dest field. */
> - if (p_size_field < size)
> + if (__compiletime_lessthan(p_size_field, size))
> __write_overflow_field(p_size_field, size);
> }
> /*
> @@ -365,25 +372,28 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
> /*
> * Length argument is a constant expression, so we
> * can perform compile-time bounds checking where
> - * buffer sizes are known.
> + * buffer sizes are also known at compile time.
> */
>
> /* Error when size is larger than enclosing struct. */
> - if (p_size > p_size_field && p_size < size)
> + if (__compiletime_lessthan(p_size_field, p_size) &&
> + __compiletime_lessthan(p_size, size))
> __write_overflow();
> - if (q_size > q_size_field && q_size < size)
> + if (__compiletime_lessthan(q_size_field, q_size) &&
> + __compiletime_lessthan(q_size, size))
> __read_overflow2();
>
> /* Warn when write size argument larger than dest field. */
> - if (p_size_field < size)
> + if (__compiletime_lessthan(p_size_field, size))
> __write_overflow_field(p_size_field, size);
> /*
> * Warn for source field over-read when building with W=1
> * or when an over-write happened, so both can be fixed at
> * the same time.
> */
> - if ((IS_ENABLED(KBUILD_EXTRA_WARN1) || p_size_field < size) &&
> - q_size_field < size)
> + if ((IS_ENABLED(KBUILD_EXTRA_WARN1) ||
> + __compiletime_lessthan(p_size_field, size)) &&
> + __compiletime_lessthan(q_size_field, size))
> __read_overflow2_field(q_size_field, size);
> }
> /*
> @@ -494,7 +504,7 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
> {
> size_t p_size = __builtin_object_size(p, 0);
>
> - if (__builtin_constant_p(size) && p_size < size)
> + if (__compiletime_lessthan(p_size, size))
> __read_overflow();
> if (p_size < size)
> fortify_panic(__func__);
> @@ -508,9 +518,9 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t
> size_t q_size = __builtin_object_size(q, 0);
>
> if (__builtin_constant_p(size)) {
> - if (p_size < size)
> + if (__compiletime_lessthan(p_size, size))
> __read_overflow();
> - if (q_size < size)
> + if (__compiletime_lessthan(q_size, size))
> __read_overflow2();
> }
> if (p_size < size || q_size < size)
> @@ -523,7 +533,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
> {
> size_t p_size = __builtin_object_size(p, 0);
>
> - if (__builtin_constant_p(size) && p_size < size)
> + if (__compiletime_lessthan(p_size, size))
> __read_overflow();
> if (p_size < size)
> fortify_panic(__func__);
> @@ -535,7 +545,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
> {
> size_t p_size = __builtin_object_size(p, 0);
>
> - if (__builtin_constant_p(size) && p_size < size)
> + if (__compiletime_lessthan(p_size, size))
> __read_overflow();
> if (p_size < size)
> fortify_panic(__func__);
> @@ -547,7 +557,7 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp
> {
> size_t p_size = __builtin_object_size(p, 0);
>
> - if (__builtin_constant_p(size) && p_size < size)
> + if (__compiletime_lessthan(p_size, size))
> __read_overflow();
> if (p_size < size)
> fortify_panic(__func__);
> @@ -563,11 +573,13 @@ char *strcpy(char * const POS p, const char * const POS q)
> size_t size;
>
> /* If neither buffer size is known, immediately give up. */
> - if (p_size == SIZE_MAX && q_size == SIZE_MAX)
> + if (__builtin_constant_p(p_size) &&
> + __builtin_constant_p(q_size) &&
> + p_size == SIZE_MAX && q_size == SIZE_MAX)
> return __underlying_strcpy(p, q);
> size = strlen(q) + 1;
> /* Compile-time check for const size overflow. */
> - if (__builtin_constant_p(size) && p_size < size)
> + if (__compiletime_lessthan(p_size, size))
> __write_overflow();
> /* Run-time check for dynamic size overflow. */
> if (p_size < size)
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available
2022-09-21 11:43 ` Siddhesh Poyarekar
@ 2022-09-22 3:33 ` Kees Cook
2022-09-22 14:45 ` Siddhesh Poyarekar
0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2022-09-22 3:33 UTC (permalink / raw)
To: Siddhesh Poyarekar
Cc: linux-hardening, Miguel Ojeda, Arnd Bergmann, Nick Desaulniers,
Nathan Chancellor, Tom Rix, llvm, Juergen Gross, Boris Ostrovsky,
linux-kernel
On Wed, Sep 21, 2022 at 07:43:17AM -0400, Siddhesh Poyarekar wrote:
> On 2022-09-20 15:22, Kees Cook wrote:
> > Since the commits starting with c37495d6254c ("slab: add __alloc_size
> > attributes for better bounds checking"), the compilers have runtime
> > allocation size hints available in some places. This was immediately
> > available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
> > updating to explicitly make use the hints via the associated
> > __builtin_dynamic_object_size() helper. Detect and use the builtin when
> > it is available, increasing the accuracy of the mitigation. When runtime
> > sizes are not available, __builtin_dynamic_object_size() falls back to
> > __builtin_object_size(), leaving the existing bounds checking unchanged.
>
> I don't know yet what the overhead is for __builtin_dynamic_object_size vs
> __builtin_object_size, were you able to measure it somehow for the kernel?
> If there's a significant tradeoff, it may make sense to provide a user
> override.
So far I've not seen any measurable performance difference, but I just
may not be creative enough yet.
So far, the tunable is building a kernel with or without FORTIFY_SOURCE
and UBSAN_BOUNDS. :)
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/4] fortify: Explicitly check bounds are compile-time constants
2022-09-21 11:48 ` Siddhesh Poyarekar
@ 2022-09-22 3:46 ` Kees Cook
0 siblings, 0 replies; 22+ messages in thread
From: Kees Cook @ 2022-09-22 3:46 UTC (permalink / raw)
To: Siddhesh Poyarekar
Cc: linux-hardening, Nathan Chancellor, Nick Desaulniers,
Arnd Bergmann, Juergen Gross, Boris Ostrovsky, Tom Rix,
Miguel Ojeda, linux-kernel, llvm
On Wed, Sep 21, 2022 at 07:48:44AM -0400, Siddhesh Poyarekar wrote:
> On 2022-09-20 15:22, Kees Cook wrote:
> > In preparation for replacing __builtin_object_size() with
> > __builtin_dynamic_object_size(), all the compile-time size checks need
> > to check that the bounds variables are, in fact, known at compile-time.
> > Enforce what was guaranteed with __bos(). In other words, since all uses
> > of __bos() were constant expressions, it was not required to test for
> > this. When these change to __bdos(), they _may_ be constant expressions,
> > and the checks are only valid when the prior condition holds. This
> > results in no binary differences.
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > include/linux/fortify-string.h | 50 +++++++++++++++++++++-------------
> > 1 file changed, 31 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> > index ff879efe94ed..71c0a432c638 100644
> > --- a/include/linux/fortify-string.h
> > +++ b/include/linux/fortify-string.h
> > @@ -80,6 +80,12 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
> > #define POS __pass_object_size(1)
> > #define POS0 __pass_object_size(0)
> > +#define __compiletime_lessthan(bounds, length) ( \
> > + __builtin_constant_p(length) && \
> > + __builtin_constant_p(bounds) && \
> > + bounds < length \
> > +)
>
> So with the gcc ranger, the compiler has lately been quite successful at
> computing a constant `bounds < length` even though bounds and length are not
> constant. So perhaps this:
>
> #define __compiletime_lessthan (bounds, length) ( \
> __builtin_constant (bounds < length) && \
> bounds < length \
> )
>
> might succeed in a few more cases.
Oh, interesting! That's very cool -- I never considered tossing a full
expression into __bcp. Yeah, that seems to work just fine:
https://godbolt.org/z/xrchErEx1
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available
2022-09-22 3:33 ` Kees Cook
@ 2022-09-22 14:45 ` Siddhesh Poyarekar
0 siblings, 0 replies; 22+ messages in thread
From: Siddhesh Poyarekar @ 2022-09-22 14:45 UTC (permalink / raw)
To: Kees Cook
Cc: linux-hardening, Miguel Ojeda, Arnd Bergmann, Nick Desaulniers,
Nathan Chancellor, Tom Rix, llvm, Juergen Gross, Boris Ostrovsky,
linux-kernel
On 2022-09-21 23:33, Kees Cook wrote:
> On Wed, Sep 21, 2022 at 07:43:17AM -0400, Siddhesh Poyarekar wrote:
>> On 2022-09-20 15:22, Kees Cook wrote:
>>> Since the commits starting with c37495d6254c ("slab: add __alloc_size
>>> attributes for better bounds checking"), the compilers have runtime
>>> allocation size hints available in some places. This was immediately
>>> available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
>>> updating to explicitly make use the hints via the associated
>>> __builtin_dynamic_object_size() helper. Detect and use the builtin when
>>> it is available, increasing the accuracy of the mitigation. When runtime
>>> sizes are not available, __builtin_dynamic_object_size() falls back to
>>> __builtin_object_size(), leaving the existing bounds checking unchanged.
>>
>> I don't know yet what the overhead is for __builtin_dynamic_object_size vs
>> __builtin_object_size, were you able to measure it somehow for the kernel?
>> If there's a significant tradeoff, it may make sense to provide a user
>> override.
>
> So far I've not seen any measurable performance difference, but I just
> may not be creative enough yet.
>
> So far, the tunable is building a kernel with or without FORTIFY_SOURCE
> and UBSAN_BOUNDS. :)
>
The overhead should only be noticeable in, e.g. fortified calls inside a
hot loop. In theory expressions to compute sizes could be arbitrarily
complex, but I haven't seen any cases yet that were large enough to be a
bother. I reckon if we find such use cases, it'll be in the kernel but
even then it may not be worth downgrading fortification across all
sources just for those specific hot paths. So I agree, the user
override isn't critically necessary.
FWIW,
Reviewed-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
Thanks,
Sid
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available
2022-09-20 19:21 [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
` (3 preceding siblings ...)
2022-09-20 19:22 ` [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
@ 2022-09-22 20:26 ` Siddhesh Poyarekar
2022-09-23 0:20 ` Kees Cook
4 siblings, 1 reply; 22+ messages in thread
From: Siddhesh Poyarekar @ 2022-09-22 20:26 UTC (permalink / raw)
To: Kees Cook, linux-hardening
Cc: Nathan Chancellor, Nick Desaulniers, Arnd Bergmann,
Juergen Gross, Boris Ostrovsky, Tom Rix, Miguel Ojeda,
linux-kernel, llvm
On 2022-09-20 15:21, Kees Cook wrote:
> Hi,
>
> This adjusts CONFIG_FORTIFY_SOURCE's coverage to include greater runtime
> size checking from GCC and Clang's __builtin_dynamic_object_size(), which
> the compilers can track either via code flow or from __alloc_size() hints.
>
FTR, I ran a linux build using gcc with allyesconfig and
fortify-metrics[1] to get a sense of how much object size coverage would
improve with __builtin_dynamic_object_size. With a total of 3,877
__builtin_object_size calls, about 11.37% succeed in getting a result
that is not (size_t)-1. If they were replaced by
__builtin_dynamic_object_size as this patch proposes, the success rate
improves to 16.25%, which is a ~1.4x improvement.
This is a decent improvement by itself but it can be amplified further
by adding __attribute__((access (...)))[2] to function prototypes and
definitions, especially for functions that take in buffers and their
sizes as arguments since __builtin_dynamic_object_size in gcc is capable
of recognizing that and using it for object size determination (and
hence to fortify calls) within those functions.
Thanks,
Sid
[1] https://github.com/siddhesh/fortify-metrics
[2] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available
2022-09-22 20:26 ` [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Siddhesh Poyarekar
@ 2022-09-23 0:20 ` Kees Cook
2022-09-23 0:55 ` Siddhesh Poyarekar
0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2022-09-23 0:20 UTC (permalink / raw)
To: Siddhesh Poyarekar
Cc: linux-hardening, Nathan Chancellor, Nick Desaulniers,
Arnd Bergmann, Juergen Gross, Boris Ostrovsky, Tom Rix,
Miguel Ojeda, linux-kernel, llvm
On Thu, Sep 22, 2022 at 04:26:54PM -0400, Siddhesh Poyarekar wrote:
> On 2022-09-20 15:21, Kees Cook wrote:
> > Hi,
> >
> > This adjusts CONFIG_FORTIFY_SOURCE's coverage to include greater runtime
> > size checking from GCC and Clang's __builtin_dynamic_object_size(), which
> > the compilers can track either via code flow or from __alloc_size() hints.
> >
>
> FTR, I ran a linux build using gcc with allyesconfig and fortify-metrics[1]
> to get a sense of how much object size coverage would improve with
> __builtin_dynamic_object_size. With a total of 3,877 __builtin_object_size
> calls, about 11.37% succeed in getting a result that is not (size_t)-1. If
> they were replaced by __builtin_dynamic_object_size as this patch proposes,
> the success rate improves to 16.25%, which is a ~1.4x improvement.
Thanks for check that! Yeah, a 40% increase in coverage is nice. :0
> This is a decent improvement by itself but it can be amplified further by
> adding __attribute__((access (...)))[2] to function prototypes and
> definitions, especially for functions that take in buffers and their sizes
> as arguments since __builtin_dynamic_object_size in gcc is capable of
> recognizing that and using it for object size determination (and hence to
> fortify calls) within those functions.
Yeah, this could be another interest set of additions. It seems like it
might be more "coder friendly" if, in the future that has the
__element_count__ attribute, it could be used in function parameters
too, like:
If we had:
int do_something(struct context *ctx, u32 *data, int count)
this seems less easy to read to me:
int __access(read_write, 2, 3) do_something(struct context *ctx, u32 *data, int count)
as this seems more readable to me, though I guess the access-mode
information is lost:
int do_something(struct context *ctx, u32 * __element_count(count) data, int count)
But yes, this would be excellent to start adding!
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available
2022-09-23 0:20 ` Kees Cook
@ 2022-09-23 0:55 ` Siddhesh Poyarekar
0 siblings, 0 replies; 22+ messages in thread
From: Siddhesh Poyarekar @ 2022-09-23 0:55 UTC (permalink / raw)
To: Kees Cook
Cc: linux-hardening, Nathan Chancellor, Nick Desaulniers,
Arnd Bergmann, Juergen Gross, Boris Ostrovsky, Tom Rix,
Miguel Ojeda, linux-kernel, llvm
On 2022-09-22 20:20, Kees Cook wrote:
> Yeah, this could be another interest set of additions. It seems like it
> might be more "coder friendly" if, in the future that has the
> __element_count__ attribute, it could be used in function parameters
> too, like:
>
> If we had:
>
> int do_something(struct context *ctx, u32 *data, int count)
>
> this seems less easy to read to me:
>
> int __access(read_write, 2, 3) do_something(struct context *ctx, u32 *data, int count)
>
> as this seems more readable to me, though I guess the access-mode
> information is lost:
>
> int do_something(struct context *ctx, u32 * __element_count(count) data, int count)
It doesn't *have* to lose access mode info:
int do_something(struct context *ctx,
u32 * __element_count(count, __read_only__) data,
int count)
{
...
}
where omitting the access mode could imply __read_write__.
Thanks,
Sid
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available
2022-09-20 19:22 ` [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
2022-09-21 11:24 ` Miguel Ojeda
2022-09-21 11:43 ` Siddhesh Poyarekar
@ 2022-11-22 10:20 ` Siddhesh Poyarekar
2022-11-23 5:15 ` Kees Cook
2023-01-13 15:59 ` linux-next - bxnt buffer overflow in strnlen Niklas Cassel
3 siblings, 1 reply; 22+ messages in thread
From: Siddhesh Poyarekar @ 2022-11-22 10:20 UTC (permalink / raw)
To: Kees Cook, linux-hardening
Cc: Miguel Ojeda, Arnd Bergmann, Nick Desaulniers, Nathan Chancellor,
Tom Rix, llvm, Juergen Gross, Boris Ostrovsky, linux-kernel
On 2022-09-20 15:22, Kees Cook wrote:
> Since the commits starting with c37495d6254c ("slab: add __alloc_size
> attributes for better bounds checking"), the compilers have runtime
> allocation size hints available in some places. This was immediately
> available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
> updating to explicitly make use the hints via the associated
> __builtin_dynamic_object_size() helper. Detect and use the builtin when
> it is available, increasing the accuracy of the mitigation. When runtime
> sizes are not available, __builtin_dynamic_object_size() falls back to
> __builtin_object_size(), leaving the existing bounds checking unchanged.
>
> Additionally update the VMALLOC_LINEAR_OVERFLOW LKDTM test to make the
> hint invisible, otherwise the architectural defense is not exercised
> (the buffer overflow is detected in the memset() rather than when it
> crosses the edge of the allocation).
>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Siddhesh Poyarekar <siddhesh@gotplt.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Tom Rix <trix@redhat.com>
> Cc: linux-hardening@vger.kernel.org
> Cc: llvm@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/misc/lkdtm/heap.c | 1 +
> include/linux/compiler_attributes.h | 5 +++++
> include/linux/fortify-string.h | 7 +++++++
> 3 files changed, 13 insertions(+)
Hi Kees,
Circling back on this, I noticed that all but this patch got pulled into
Linus' tree. Is there a reason why this has been held back?
Thanks,
Sid
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available
2022-11-22 10:20 ` Siddhesh Poyarekar
@ 2022-11-23 5:15 ` Kees Cook
2022-11-23 15:29 ` Siddhesh Poyarekar
0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2022-11-23 5:15 UTC (permalink / raw)
To: Siddhesh Poyarekar
Cc: linux-hardening, Miguel Ojeda, Arnd Bergmann, Nick Desaulniers,
Nathan Chancellor, Tom Rix, llvm, Juergen Gross, Boris Ostrovsky,
linux-kernel
On Tue, Nov 22, 2022 at 05:20:37AM -0500, Siddhesh Poyarekar wrote:
> On 2022-09-20 15:22, Kees Cook wrote:
> > Since the commits starting with c37495d6254c ("slab: add __alloc_size
> > attributes for better bounds checking"), the compilers have runtime
> > allocation size hints available in some places. This was immediately
> > available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
> > updating to explicitly make use the hints via the associated
> > __builtin_dynamic_object_size() helper. Detect and use the builtin when
> > it is available, increasing the accuracy of the mitigation. When runtime
> > sizes are not available, __builtin_dynamic_object_size() falls back to
> > __builtin_object_size(), leaving the existing bounds checking unchanged.
> >
> > Additionally update the VMALLOC_LINEAR_OVERFLOW LKDTM test to make the
> > hint invisible, otherwise the architectural defense is not exercised
> > (the buffer overflow is detected in the memset() rather than when it
> > crosses the edge of the allocation).
> >
> > Cc: Miguel Ojeda <ojeda@kernel.org>
> > Cc: Siddhesh Poyarekar <siddhesh@gotplt.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Nathan Chancellor <nathan@kernel.org>
> > Cc: Tom Rix <trix@redhat.com>
> > Cc: linux-hardening@vger.kernel.org
> > Cc: llvm@lists.linux.dev
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > drivers/misc/lkdtm/heap.c | 1 +
> > include/linux/compiler_attributes.h | 5 +++++
> > include/linux/fortify-string.h | 7 +++++++
> > 3 files changed, 13 insertions(+)
>
> Hi Kees,
>
> Circling back on this, I noticed that all but this patch got pulled into
> Linus' tree. Is there a reason why this has been held back?
Hi!
Yeah, it depended on a bunch of various clean-ups, which have finally
managed to land. It's late enough in the devel cycle that I suspect I
will hold this one back until after the merge window and then make sure
it has plenty of time to bake in -next. If the rest of the patches
continue to behave, I may change my mind... :)
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available
2022-11-23 5:15 ` Kees Cook
@ 2022-11-23 15:29 ` Siddhesh Poyarekar
0 siblings, 0 replies; 22+ messages in thread
From: Siddhesh Poyarekar @ 2022-11-23 15:29 UTC (permalink / raw)
To: Kees Cook
Cc: linux-hardening, Miguel Ojeda, Arnd Bergmann, Nick Desaulniers,
Nathan Chancellor, Tom Rix, llvm, Juergen Gross, Boris Ostrovsky,
linux-kernel
On 2022-11-23 00:15, Kees Cook wrote:
> Yeah, it depended on a bunch of various clean-ups, which have finally
> managed to land. It's late enough in the devel cycle that I suspect I
> will hold this one back until after the merge window and then make sure
> it has plenty of time to bake in -next. If the rest of the patches
> continue to behave, I may change my mind... :)
OK, thanks for responding!
Sid
^ permalink raw reply [flat|nested] 22+ messages in thread
* linux-next - bxnt buffer overflow in strnlen
2022-09-20 19:22 ` [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
` (2 preceding siblings ...)
2022-11-22 10:20 ` Siddhesh Poyarekar
@ 2023-01-13 15:59 ` Niklas Cassel
2023-01-13 16:08 ` linux-next - bnxt " Niklas Cassel
3 siblings, 1 reply; 22+ messages in thread
From: Niklas Cassel @ 2023-01-13 15:59 UTC (permalink / raw)
To: Kees Cook
Cc: linux-hardening, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann,
Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm,
Juergen Gross, Boris Ostrovsky, linux-kernel, Damien Le Moal,
linux-next
On Tue, Sep 20, 2022 at 12:22:02PM -0700, Kees Cook wrote:
> Since the commits starting with c37495d6254c ("slab: add __alloc_size
> attributes for better bounds checking"), the compilers have runtime
> allocation size hints available in some places. This was immediately
> available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
> updating to explicitly make use the hints via the associated
> __builtin_dynamic_object_size() helper. Detect and use the builtin when
> it is available, increasing the accuracy of the mitigation. When runtime
> sizes are not available, __builtin_dynamic_object_size() falls back to
> __builtin_object_size(), leaving the existing bounds checking unchanged.
>
> Additionally update the VMALLOC_LINEAR_OVERFLOW LKDTM test to make the
> hint invisible, otherwise the architectural defense is not exercised
> (the buffer overflow is detected in the memset() rather than when it
> crosses the edge of the allocation).
>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Siddhesh Poyarekar <siddhesh@gotplt.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Tom Rix <trix@redhat.com>
> Cc: linux-hardening@vger.kernel.org
> Cc: llvm@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Hello Kees,
Unfortunately, this commit introduces a crash in the bxnt
ethernet driver when booting linux-next.
I haven't looked at the code in the bxnt ethernet driver,
I simply know that machine boots fine on v6.2.0-rc3,
but fails to boot with linux-next.
So I started an automatic git bisect, which returned:
439a1bcac648 ("fortify: Use __builtin_dynamic_object_size() when available")
$ grep CC_VERSION .config
CONFIG_CC_VERSION_TEXT="gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)"
CONFIG_GCC_VERSION=120201
$ grep FORTIFY .config
CONFIG_ARCH_HAS_FORTIFY_SOURCE=y
CONFIG_FORTIFY_SOURCE=y
dmesg output:
<0>[ 10.805253] detected buffer overflow in strnlen
<4>[ 10.810683] ------------[ cut here ]------------
<2>[ 10.816035] kernel BUG at lib/string_helpers.c:1027!
<4>[ 10.821753] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
<4>[ 10.822737] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc3-next-20230112+ #4
<4>[ 10.834787] Hardware name: Supermicro Super Server/H12SSL-NT, BIOS 2.4 04/14/2022
<4>[ 10.839875] RIP: 0010:fortify_panic+0xf/0x11
<4>[ 10.844962] Code: e0 e8 da 83 0a ff e9 31 45 6d ff 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 48 89 fe 48 c7 c7 78 69 d6 9f e8 01 ea fc ff <0f> 0b 48 8b 4c 24 18 48 8b 54 24 10 4c 8d 44 24 25 48 c7 c7 b6 69
<4>[ 10.865321] RSP: 0018:ffffb547c005bb98 EFLAGS: 00010246
<4>[ 10.870406] RAX: 0000000000000023 RBX: ffff94f0582bc400 RCX: 0000000000000000
<4>[ 10.880584] RDX: 0000000000000001 RSI: 00000000ffffdfff RDI: 00000000ffffffff
<4>[ 10.885672] RBP: ffff94f0582bc424 R08: 0000000000000000 R09: ffffb547c005ba60
<4>[ 10.895849] R10: 0000000000000003 R11: ffffffffa0182448 R12: 696c66666f282074
<4>[ 10.900937] R13: 736574206b636162 R14: 0000000000000001 R15: ffff94f0545f8b40
<4>[ 10.911113] FS: 0000000000000000(0000) GS:ffff950f07380000(0000) knlGS:0000000000000000
<4>[ 10.916201] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[ 10.926382] CR2: 0000000000000000 CR3: 000000204c05a000 CR4: 0000000000350ee0
<4>[ 10.931470] Call Trace:
<6>[ 10.936317] ata9: SATA link down (SStatus 0 SControl 300)
<4>[ 10.936745] <TASK>
<4>[ 10.936745] bnxt_ethtool_init.cold+0x18/0x18
<4>[ 10.936745] ? dma_pool_free+0x14d/0x160
<6>[ 10.942591] ata10: SATA link down (SStatus 0 SControl 300)
<4>[ 10.942663] bnxt_fw_init_one_p2+0x18d/0x5e0
<6>[ 10.949046] ata4: SATA link down (SStatus 0 SControl 300)
<4>[ 10.949841] bnxt_init_one+0x401/0xf10
<6>[ 10.958451] ata6: SATA link down (SStatus 0 SControl 300)
<4>[ 10.958854] local_pci_probe+0x41/0x80
<6>[ 10.968114] ata3: SATA link down (SStatus 0 SControl 300)
<4>[ 10.968892] pci_device_probe+0x1e2/0x210
<6>[ 10.977259] ata8: SATA link down (SStatus 0 SControl 300)
<4>[ 10.977657] really_probe+0xde/0x380
<6>[ 10.986406] ata5: SATA link down (SStatus 0 SControl 300)
<4>[ 10.986817] ? pm_runtime_barrier+0x50/0x90
<4>[ 10.986817] __driver_probe_device+0x78/0x170
<6>[ 10.996042] ata7: SATA link down (SStatus 0 SControl 300)
<4>[ 10.996978] driver_probe_device+0x1f/0x90
<4>[ 10.996978] __driver_attach+0xd2/0x1c0
<4>[ 10.996978] ? __pfx___driver_attach+0x10/0x10
<4>[ 10.996978] bus_for_each_dev+0x65/0x90
<4>[ 11.047368] bus_add_driver+0x1b1/0x200
<4>[ 11.052640] driver_register+0x89/0xe0
<4>[ 11.057487] ? __pfx_bnxt_init+0x10/0x10
<4>[ 11.061634] bnxt_init+0x20/0x33
<4>[ 11.065015] do_one_initcall+0x5b/0x340
<4>[ 11.070105] ? rcu_read_lock_sched_held+0x3f/0x80
<4>[ 11.075198] kernel_init_freeable+0x29e/0x2ee
<4>[ 11.080635] ? __pfx_kernel_init+0x10/0x10
<4>[ 11.085379] kernel_init+0x16/0x140
<6>[ 11.087743] ata16: SATA link down (SStatus 0 SControl 300)
<4>[ 11.086528] ret_from_fork+0x2c/0x50
<4>[ 11.086528] </TASK>
<6>[ 11.094437] ata17: SATA link down (SStatus 0 SControl 300)
<4>[ 11.094645] Modules linked in:
<4>[ 11.097999] ---[ end trace 0000000000000000 ]---
<6>[ 11.100194] ata18: SATA link down (SStatus 0 SControl 300)
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next - bnxt buffer overflow in strnlen
2023-01-13 15:59 ` linux-next - bxnt buffer overflow in strnlen Niklas Cassel
@ 2023-01-13 16:08 ` Niklas Cassel
2023-01-13 22:44 ` Kees Cook
0 siblings, 1 reply; 22+ messages in thread
From: Niklas Cassel @ 2023-01-13 16:08 UTC (permalink / raw)
To: Kees Cook
Cc: linux-hardening, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann,
Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm,
Juergen Gross, Boris Ostrovsky, linux-kernel, Damien Le Moal,
linux-next, netdev, Michael Chan
On Fri, Jan 13, 2023 at 04:59:19PM +0100, Niklas Cassel wrote:
> On Tue, Sep 20, 2022 at 12:22:02PM -0700, Kees Cook wrote:
> > Since the commits starting with c37495d6254c ("slab: add __alloc_size
> > attributes for better bounds checking"), the compilers have runtime
> > allocation size hints available in some places. This was immediately
> > available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
> > updating to explicitly make use the hints via the associated
> > __builtin_dynamic_object_size() helper. Detect and use the builtin when
> > it is available, increasing the accuracy of the mitigation. When runtime
> > sizes are not available, __builtin_dynamic_object_size() falls back to
> > __builtin_object_size(), leaving the existing bounds checking unchanged.
> >
> > Additionally update the VMALLOC_LINEAR_OVERFLOW LKDTM test to make the
> > hint invisible, otherwise the architectural defense is not exercised
> > (the buffer overflow is detected in the memset() rather than when it
> > crosses the edge of the allocation).
> >
> > Cc: Miguel Ojeda <ojeda@kernel.org>
> > Cc: Siddhesh Poyarekar <siddhesh@gotplt.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Nathan Chancellor <nathan@kernel.org>
> > Cc: Tom Rix <trix@redhat.com>
> > Cc: linux-hardening@vger.kernel.org
> > Cc: llvm@lists.linux.dev
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
>
> Hello Kees,
>
> Unfortunately, this commit introduces a crash in the bnxt
> ethernet driver when booting linux-next.
>
> I haven't looked at the code in the bnxt ethernet driver,
> I simply know that machine boots fine on v6.2.0-rc3,
> but fails to boot with linux-next.
>
> So I started an automatic git bisect, which returned:
> 439a1bcac648 ("fortify: Use __builtin_dynamic_object_size() when available")
>
> $ grep CC_VERSION .config
> CONFIG_CC_VERSION_TEXT="gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)"
> CONFIG_GCC_VERSION=120201
>
> $ grep FORTIFY .config
> CONFIG_ARCH_HAS_FORTIFY_SOURCE=y
> CONFIG_FORTIFY_SOURCE=y
>
>
> dmesg output:
>
> <0>[ 10.805253] detected buffer overflow in strnlen
> <4>[ 10.810683] ------------[ cut here ]------------
> <2>[ 10.816035] kernel BUG at lib/string_helpers.c:1027!
> <4>[ 10.821753] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> <4>[ 10.822737] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc3-next-20230112+ #4
> <4>[ 10.834787] Hardware name: Supermicro Super Server/H12SSL-NT, BIOS 2.4 04/14/2022
> <4>[ 10.839875] RIP: 0010:fortify_panic+0xf/0x11
> <4>[ 10.844962] Code: e0 e8 da 83 0a ff e9 31 45 6d ff 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 48 89 fe 48 c7 c7 78 69 d6 9f e8 01 ea fc ff <0f> 0b 48 8b 4c 24 18 48 8b 54 24 10 4c 8d 44 24 25 48 c7 c7 b6 69
> <4>[ 10.865321] RSP: 0018:ffffb547c005bb98 EFLAGS: 00010246
> <4>[ 10.870406] RAX: 0000000000000023 RBX: ffff94f0582bc400 RCX: 0000000000000000
> <4>[ 10.880584] RDX: 0000000000000001 RSI: 00000000ffffdfff RDI: 00000000ffffffff
> <4>[ 10.885672] RBP: ffff94f0582bc424 R08: 0000000000000000 R09: ffffb547c005ba60
> <4>[ 10.895849] R10: 0000000000000003 R11: ffffffffa0182448 R12: 696c66666f282074
> <4>[ 10.900937] R13: 736574206b636162 R14: 0000000000000001 R15: ffff94f0545f8b40
> <4>[ 10.911113] FS: 0000000000000000(0000) GS:ffff950f07380000(0000) knlGS:0000000000000000
> <4>[ 10.916201] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>[ 10.926382] CR2: 0000000000000000 CR3: 000000204c05a000 CR4: 0000000000350ee0
> <4>[ 10.931470] Call Trace:
> <6>[ 10.936317] ata9: SATA link down (SStatus 0 SControl 300)
> <4>[ 10.936745] <TASK>
> <4>[ 10.936745] bnxt_ethtool_init.cold+0x18/0x18
> <4>[ 10.936745] ? dma_pool_free+0x14d/0x160
> <6>[ 10.942591] ata10: SATA link down (SStatus 0 SControl 300)
> <4>[ 10.942663] bnxt_fw_init_one_p2+0x18d/0x5e0
> <6>[ 10.949046] ata4: SATA link down (SStatus 0 SControl 300)
> <4>[ 10.949841] bnxt_init_one+0x401/0xf10
> <6>[ 10.958451] ata6: SATA link down (SStatus 0 SControl 300)
> <4>[ 10.958854] local_pci_probe+0x41/0x80
> <6>[ 10.968114] ata3: SATA link down (SStatus 0 SControl 300)
> <4>[ 10.968892] pci_device_probe+0x1e2/0x210
> <6>[ 10.977259] ata8: SATA link down (SStatus 0 SControl 300)
> <4>[ 10.977657] really_probe+0xde/0x380
> <6>[ 10.986406] ata5: SATA link down (SStatus 0 SControl 300)
> <4>[ 10.986817] ? pm_runtime_barrier+0x50/0x90
> <4>[ 10.986817] __driver_probe_device+0x78/0x170
> <6>[ 10.996042] ata7: SATA link down (SStatus 0 SControl 300)
> <4>[ 10.996978] driver_probe_device+0x1f/0x90
> <4>[ 10.996978] __driver_attach+0xd2/0x1c0
> <4>[ 10.996978] ? __pfx___driver_attach+0x10/0x10
> <4>[ 10.996978] bus_for_each_dev+0x65/0x90
> <4>[ 11.047368] bus_add_driver+0x1b1/0x200
> <4>[ 11.052640] driver_register+0x89/0xe0
> <4>[ 11.057487] ? __pfx_bnxt_init+0x10/0x10
> <4>[ 11.061634] bnxt_init+0x20/0x33
> <4>[ 11.065015] do_one_initcall+0x5b/0x340
> <4>[ 11.070105] ? rcu_read_lock_sched_held+0x3f/0x80
> <4>[ 11.075198] kernel_init_freeable+0x29e/0x2ee
> <4>[ 11.080635] ? __pfx_kernel_init+0x10/0x10
> <4>[ 11.085379] kernel_init+0x16/0x140
> <6>[ 11.087743] ata16: SATA link down (SStatus 0 SControl 300)
> <4>[ 11.086528] ret_from_fork+0x2c/0x50
> <4>[ 11.086528] </TASK>
> <6>[ 11.094437] ata17: SATA link down (SStatus 0 SControl 300)
> <4>[ 11.094645] Modules linked in:
> <4>[ 11.097999] ---[ end trace 0000000000000000 ]---
> <6>[ 11.100194] ata18: SATA link down (SStatus 0 SControl 300)
>
>
> Kind regards,
> Niklas
+netdev
+bnxt maintainers
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next - bnxt buffer overflow in strnlen
2023-01-13 16:08 ` linux-next - bnxt " Niklas Cassel
@ 2023-01-13 22:44 ` Kees Cook
2023-01-16 10:56 ` Niklas Cassel
0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2023-01-13 22:44 UTC (permalink / raw)
To: Niklas Cassel
Cc: linux-hardening, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann,
Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm,
Juergen Gross, Boris Ostrovsky, linux-kernel, Damien Le Moal,
linux-next, netdev, Michael Chan
On Fri, Jan 13, 2023 at 04:08:21PM +0000, Niklas Cassel wrote:
> On Fri, Jan 13, 2023 at 04:59:19PM +0100, Niklas Cassel wrote:
> > On Tue, Sep 20, 2022 at 12:22:02PM -0700, Kees Cook wrote:
> > > Since the commits starting with c37495d6254c ("slab: add __alloc_size
> > > attributes for better bounds checking"), the compilers have runtime
> > > allocation size hints available in some places. This was immediately
> > > available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
> > > updating to explicitly make use the hints via the associated
> > > __builtin_dynamic_object_size() helper. Detect and use the builtin when
> > > it is available, increasing the accuracy of the mitigation. When runtime
> > > sizes are not available, __builtin_dynamic_object_size() falls back to
> > > __builtin_object_size(), leaving the existing bounds checking unchanged.
> > > [...]
> > Hello Kees,
> >
> > Unfortunately, this commit introduces a crash in the bnxt
> > ethernet driver when booting linux-next.
Hi! Thanks for the report. Notes below...
> > I haven't looked at the code in the bnxt ethernet driver,
> > I simply know that machine boots fine on v6.2.0-rc3,
> > but fails to boot with linux-next.
> >
> > So I started an automatic git bisect, which returned:
> > 439a1bcac648 ("fortify: Use __builtin_dynamic_object_size() when available")
> >
> > $ grep CC_VERSION .config
> > CONFIG_CC_VERSION_TEXT="gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)"
> > CONFIG_GCC_VERSION=120201
> >
> > $ grep FORTIFY .config
> > CONFIG_ARCH_HAS_FORTIFY_SOURCE=y
> > CONFIG_FORTIFY_SOURCE=y
> >
> >
> > dmesg output:
> >
> > <0>[ 10.805253] detected buffer overflow in strnlen
> [...]
> > <4>[ 10.931470] Call Trace:
> > <6>[ 10.936317] ata9: SATA link down (SStatus 0 SControl 300)
> > <4>[ 10.936745] <TASK>
> > <4>[ 10.936745] bnxt_ethtool_init.cold+0x18/0x18
Are you able to run:
$ ./scripts/faddr2line vmlinux bnxt_ethtool_init.cold+0x18/0x18
to find the exact line it's failing on, just to be sure we're looking in
the right place?
There are a bunch of string functions being used in a loop
bnxt_ethtool_init(). Here's the code:
if (bp->num_tests > BNXT_MAX_TEST)
bp->num_tests = BNXT_MAX_TEST;
...
for (i = 0; i < bp->num_tests; i++) {
char *str = test_info->string[i];
char *fw_str = resp->test0_name + i * 32;
if (i == BNXT_MACLPBK_TEST_IDX) {
strcpy(str, "Mac loopback test (offline)");
} else if (i == BNXT_PHYLPBK_TEST_IDX) {
strcpy(str, "Phy loopback test (offline)");
} else if (i == BNXT_EXTLPBK_TEST_IDX) {
strcpy(str, "Ext loopback test (offline)");
} else if (i == BNXT_IRQ_TEST_IDX) {
strcpy(str, "Interrupt_test (offline)");
} else {
strscpy(str, fw_str, ETH_GSTRING_LEN);
strncat(str, " test", ETH_GSTRING_LEN - strlen(str));
if (test_info->offline_mask & (1 << i))
strncat(str, " (offline)",
ETH_GSTRING_LEN - strlen(str));
else
strncat(str, " (online)",
ETH_GSTRING_LEN - strlen(str));
}
}
The hardened strnlen() is used internally to the hardened strcpy() and
strscpy()'s source argument, and strncat()'s dest and source arguments.
The only non-literal source argument is fw_str.
The destination in this loop is always "str", which is test_info->string[i].
I'd expect "str" to always be processed as fixed size:
struct bnxt_test_info {
u8 offline_mask;
u16 timeout;
char string[BNXT_MAX_TEST][ETH_GSTRING_LEN];
};
#define ETH_GSTRING_LEN 32
#define BNXT_MAX_TEST 8
And the allocation matches that size:
test_info = kzalloc(sizeof(*bp->test_info), GFP_KERNEL);
(bp->test_info is, indeed struct bnxt_test_info too.)
The loop cannot reach BNXT_MAX_TEST. It looks like fw_str's size isn't
known dynamically, so that shouldn't be a change. (It's assigned from
a void * return.) So I suspect "str" ran off the end of the allocation,
which implies that "fw_str" must be >= ETH_GSTRING_LEN. This line looks
very suspicious:
char *fw_str = resp->test0_name + i * 32;
I also note that the return value of strscpy() is not checked...
Let's see...
struct hwrm_selftest_qlist_output {
...
char test0_name[32];
char test1_name[32];
char test2_name[32];
char test3_name[32];
char test4_name[32];
char test5_name[32];
char test6_name[32];
char test7_name[32];
...
};
Ew. So, yes, it's specifically reach past the end of the test0_name[]
array, *and* is may overflow the heap. Does this patch solve it for you?
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index cbf17fcfb7ab..ec573127b707 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -3969,7 +3969,7 @@ void bnxt_ethtool_init(struct bnxt *bp)
test_info->timeout = HWRM_CMD_TIMEOUT;
for (i = 0; i < bp->num_tests; i++) {
char *str = test_info->string[i];
- char *fw_str = resp->test0_name + i * 32;
+ char *fw_str = resp->test_name[i];
if (i == BNXT_MACLPBK_TEST_IDX) {
strcpy(str, "Mac loopback test (offline)");
@@ -3980,14 +3980,9 @@ void bnxt_ethtool_init(struct bnxt *bp)
} else if (i == BNXT_IRQ_TEST_IDX) {
strcpy(str, "Interrupt_test (offline)");
} else {
- strscpy(str, fw_str, ETH_GSTRING_LEN);
- strncat(str, " test", ETH_GSTRING_LEN - strlen(str));
- if (test_info->offline_mask & (1 << i))
- strncat(str, " (offline)",
- ETH_GSTRING_LEN - strlen(str));
- else
- strncat(str, " (online)",
- ETH_GSTRING_LEN - strlen(str));
+ snprintf(str, ETH_GSTRING_LEN, "%s test (%s)",
+ fw_str, test_info->offline_mask & (1 << i) ?
+ "offline" : "online");
}
}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
index 2686a714a59f..a5408879e077 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
@@ -10249,14 +10249,7 @@ struct hwrm_selftest_qlist_output {
u8 unused_0;
__le16 test_timeout;
u8 unused_1[2];
- char test0_name[32];
- char test1_name[32];
- char test2_name[32];
- char test3_name[32];
- char test4_name[32];
- char test5_name[32];
- char test6_name[32];
- char test7_name[32];
+ char test_name[8][32];
u8 eyescope_target_BER_support;
#define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E8_SUPPORTED 0x0UL
#define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E9_SUPPORTED 0x1UL
--
Kees Cook
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: linux-next - bnxt buffer overflow in strnlen
2023-01-13 22:44 ` Kees Cook
@ 2023-01-16 10:56 ` Niklas Cassel
0 siblings, 0 replies; 22+ messages in thread
From: Niklas Cassel @ 2023-01-16 10:56 UTC (permalink / raw)
To: Kees Cook
Cc: linux-hardening, Miguel Ojeda, Siddhesh Poyarekar, Arnd Bergmann,
Nick Desaulniers, Nathan Chancellor, Tom Rix, llvm,
Juergen Gross, Boris Ostrovsky, linux-kernel, Damien Le Moal,
linux-next, netdev, Michael Chan
On Fri, Jan 13, 2023 at 02:44:32PM -0800, Kees Cook wrote:
> On Fri, Jan 13, 2023 at 04:08:21PM +0000, Niklas Cassel wrote:
> > > Hello Kees,
> > >
> > > Unfortunately, this commit introduces a crash in the bnxt
> > > ethernet driver when booting linux-next.
(snip)
>
> Let's see...
>
> struct hwrm_selftest_qlist_output {
> ...
> char test0_name[32];
> char test1_name[32];
> char test2_name[32];
> char test3_name[32];
> char test4_name[32];
> char test5_name[32];
> char test6_name[32];
> char test7_name[32];
> ...
> };
>
> Ew. So, yes, it's specifically reach past the end of the test0_name[]
> array, *and* is may overflow the heap. Does this patch solve it for you?
Yes, it does!
Thank you very much Kees, both for this patch, and for all the excellent work
that you've done with regard to kernel hardening in general over the years.
Feel free to add my:
Tested-by: Niklas Cassel <niklas.cassel@wdc.com>
if you send out a real patch.
Kind regards,
Niklas
>
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index cbf17fcfb7ab..ec573127b707 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -3969,7 +3969,7 @@ void bnxt_ethtool_init(struct bnxt *bp)
> test_info->timeout = HWRM_CMD_TIMEOUT;
> for (i = 0; i < bp->num_tests; i++) {
> char *str = test_info->string[i];
> - char *fw_str = resp->test0_name + i * 32;
> + char *fw_str = resp->test_name[i];
>
> if (i == BNXT_MACLPBK_TEST_IDX) {
> strcpy(str, "Mac loopback test (offline)");
> @@ -3980,14 +3980,9 @@ void bnxt_ethtool_init(struct bnxt *bp)
> } else if (i == BNXT_IRQ_TEST_IDX) {
> strcpy(str, "Interrupt_test (offline)");
> } else {
> - strscpy(str, fw_str, ETH_GSTRING_LEN);
> - strncat(str, " test", ETH_GSTRING_LEN - strlen(str));
> - if (test_info->offline_mask & (1 << i))
> - strncat(str, " (offline)",
> - ETH_GSTRING_LEN - strlen(str));
> - else
> - strncat(str, " (online)",
> - ETH_GSTRING_LEN - strlen(str));
> + snprintf(str, ETH_GSTRING_LEN, "%s test (%s)",
> + fw_str, test_info->offline_mask & (1 << i) ?
> + "offline" : "online");
> }
> }
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> index 2686a714a59f..a5408879e077 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> @@ -10249,14 +10249,7 @@ struct hwrm_selftest_qlist_output {
> u8 unused_0;
> __le16 test_timeout;
> u8 unused_1[2];
> - char test0_name[32];
> - char test1_name[32];
> - char test2_name[32];
> - char test3_name[32];
> - char test4_name[32];
> - char test5_name[32];
> - char test6_name[32];
> - char test7_name[32];
> + char test_name[8][32];
> u8 eyescope_target_BER_support;
> #define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E8_SUPPORTED 0x0UL
> #define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E9_SUPPORTED 0x1UL
>
>
>
>
>
> --
> Kees Cook
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-01-16 10:57 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-20 19:21 [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
2022-09-20 19:21 ` [PATCH 1/4] x86/entry: Work around Clang __bdos() bug Kees Cook
2022-09-21 0:07 ` Boris Ostrovsky
2022-09-20 19:22 ` [PATCH 2/4] fortify: Explicitly check bounds are compile-time constants Kees Cook
2022-09-21 11:48 ` Siddhesh Poyarekar
2022-09-22 3:46 ` Kees Cook
2022-09-20 19:22 ` [PATCH 3/4] fortify: Convert to struct vs member helpers Kees Cook
2022-09-20 19:22 ` [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available Kees Cook
2022-09-21 11:24 ` Miguel Ojeda
2022-09-21 11:43 ` Siddhesh Poyarekar
2022-09-22 3:33 ` Kees Cook
2022-09-22 14:45 ` Siddhesh Poyarekar
2022-11-22 10:20 ` Siddhesh Poyarekar
2022-11-23 5:15 ` Kees Cook
2022-11-23 15:29 ` Siddhesh Poyarekar
2023-01-13 15:59 ` linux-next - bxnt buffer overflow in strnlen Niklas Cassel
2023-01-13 16:08 ` linux-next - bnxt " Niklas Cassel
2023-01-13 22:44 ` Kees Cook
2023-01-16 10:56 ` Niklas Cassel
2022-09-22 20:26 ` [PATCH 0/4] fortify: Use __builtin_dynamic_object_size() when available Siddhesh Poyarekar
2022-09-23 0:20 ` Kees Cook
2022-09-23 0:55 ` Siddhesh Poyarekar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).