diff mbox series

fortify: Improve buffer overflow reporting

Message ID 20230302225808.never.375-kees@kernel.org (mailing list archive)
State New, archived
Headers show
Series fortify: Improve buffer overflow reporting | expand

Commit Message

Kees Cook March 2, 2023, 10:58 p.m. UTC 
Improve the reporting of buffer overflows under CONFIG_FORTIFY_SOURCE to
help accelerate debugging efforts. The calculations are all just sitting
in registers anyway, so pass them along to the function to be reported.

For example, before:

  detected buffer overflow in memcpy

and after:

  memcpy: detected buffer overflow: 4096 byte read from buffer of size 1

Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Andy Shevchenko <andy@kernel.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nicolas Schier <nicolas@fjasle.eu>
Cc: Tom Rix <trix@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Miroslav Benes <mbenes@suse.cz>
Cc: Marco Elver <elver@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Puyou Lu <puyou.lu@gmail.com>
Cc: linux-hardening@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/fortify-string.h | 51 +++++++++++++++++++---------------
 lib/string_helpers.c           |  5 ++--
 2 files changed, 32 insertions(+), 24 deletions(-)

Comments

Nick Desaulniers March 2, 2023, 11:21 p.m. UTC | #1 
On Thu, Mar 2, 2023 at 2:58 PM Kees Cook <keescook@chromium.org> wrote:
>
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index c9de1f59ee80..981e2838f99a 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -170,11 +170,13 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
>  char *strcat(char * const POS p, const char *q)
>  {
>         size_t p_size = __member_size(p);
> +       size_t size;
>
>         if (p_size == SIZE_MAX)
>                 return __underlying_strcat(p, q);
> -       if (strlcat(p, q, p_size) >= p_size)
> -               fortify_panic(__func__);
> +       size = strlcat(p, q, p_size);
> +       if (p_size < size)

What happens when they're equal? I think this patch changes
behavior...? Intentional?

Did flipping this conditional drop what should be `<=`?

Was there an off by one, or is this version of this patch potentially
introducing one? Or am I misremembering my boolean algebra?

> +               fortify_panic(__func__, 1, p_size, size);
>         return p;
>  }
>
> @@ -205,7 +207,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
>         /* Do not check characters beyond the end of p. */
>         ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
>         if (p_size <= ret && maxlen != ret)
> -               fortify_panic(__func__);
> +               fortify_panic(__func__, 1, p_size, ret);
>         return ret;
>  }
>
> @@ -241,7 +243,7 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
>                 return __underlying_strlen(p);
>         ret = strnlen(p, p_size);
>         if (p_size <= ret)
> -               fortify_panic(__func__);
> +               fortify_panic(__func__, 1, p_size, ret);
>         return ret;
>  }
>
> @@ -282,8 +284,8 @@ __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, si
>                         __write_overflow();
>         }
>         if (size) {
> -               if (len >= p_size)
> -                       fortify_panic(__func__);
> +               if (p_size < len)

`<=` ? (This used to panic when they were equal)

> +                       fortify_panic(__func__, 1, p_size, len);
>                 __underlying_memcpy(p, q, len);
>                 p[len] = '\0';
>         }
Kees Cook March 3, 2023, 1:37 a.m. UTC | #2 
On March 2, 2023 3:21:11 PM PST, Nick Desaulniers <ndesaulniers@google.com> wrote:
>On Thu, Mar 2, 2023 at 2:58 PM Kees Cook <keescook@chromium.org> wrote:
>>
>> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
>> index c9de1f59ee80..981e2838f99a 100644
>> --- a/include/linux/fortify-string.h
>> +++ b/include/linux/fortify-string.h
>> @@ -170,11 +170,13 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
>>  char *strcat(char * const POS p, const char *q)
>>  {
>>         size_t p_size = __member_size(p);
>> +       size_t size;
>>
>>         if (p_size == SIZE_MAX)
>>                 return __underlying_strcat(p, q);
>> -       if (strlcat(p, q, p_size) >= p_size)
>> -               fortify_panic(__func__);
>> +       size = strlcat(p, q, p_size);
>> +       if (p_size < size)
>
>What happens when they're equal? I think this patch changes
>behavior...? Intentional?
>
>Did flipping this conditional drop what should be `<=`?
>
>Was there an off by one, or is this version of this patch potentially
>introducing one? Or am I misremembering my boolean algebra?

Whoops! Thanks for catching that. I was going too fast. And I'm bothered that my regression tests missed it. :|

I will send a v2...

-Kees
diff mbox series

Patch

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index c9de1f59ee80..981e2838f99a 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -9,7 +9,7 @@ 
 #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
 #define __RENAME(x) __asm__(#x)
 
-void fortify_panic(const char *name) __noreturn __cold;
+void fortify_panic(const char *name, bool dest, size_t avail, size_t len) __noreturn __cold;
 void __read_overflow(void) __compiletime_error("detected read beyond size of object (1st parameter)");
 void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
 void __read_overflow2_field(size_t avail, size_t wanted) __compiletime_warning("detected read beyond size of field (2nd parameter); maybe use struct_group()?");
@@ -147,7 +147,7 @@  char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__write_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(__func__, 1, p_size, size);
 	return __underlying_strncpy(p, q, size);
 }
 
@@ -170,11 +170,13 @@  __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
 char *strcat(char * const POS p, const char *q)
 {
 	size_t p_size = __member_size(p);
+	size_t size;
 
 	if (p_size == SIZE_MAX)
 		return __underlying_strcat(p, q);
-	if (strlcat(p, q, p_size) >= p_size)
-		fortify_panic(__func__);
+	size = strlcat(p, q, p_size);
+	if (p_size < size)
+		fortify_panic(__func__, 1, p_size, size);
 	return p;
 }
 
@@ -205,7 +207,7 @@  __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
 	/* Do not check characters beyond the end of p. */
 	ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
 	if (p_size <= ret && maxlen != ret)
-		fortify_panic(__func__);
+		fortify_panic(__func__, 1, p_size, ret);
 	return ret;
 }
 
@@ -241,7 +243,7 @@  __kernel_size_t __fortify_strlen(const char * const POS p)
 		return __underlying_strlen(p);
 	ret = strnlen(p, p_size);
 	if (p_size <= ret)
-		fortify_panic(__func__);
+		fortify_panic(__func__, 1, p_size, ret);
 	return ret;
 }
 
@@ -282,8 +284,8 @@  __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, si
 			__write_overflow();
 	}
 	if (size) {
-		if (len >= p_size)
-			fortify_panic(__func__);
+		if (p_size < len)
+			fortify_panic(__func__, 1, p_size, len);
 		__underlying_memcpy(p, q, len);
 		p[len] = '\0';
 	}
@@ -361,7 +363,7 @@  __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
 	 * p_size.
 	 */
 	if (len > p_size)
-		fortify_panic(__func__);
+		fortify_panic(__func__, 1, p_size, len);
 
 	/*
 	 * We can now safely call vanilla strscpy because we are protected from:
@@ -397,13 +399,15 @@  char *strncat(char * const POS p, const char * const POS q, __kernel_size_t coun
 	size_t p_len, copy_len;
 	size_t p_size = __member_size(p);
 	size_t q_size = __member_size(q);
+	size_t size;
 
 	if (p_size == SIZE_MAX && q_size == SIZE_MAX)
 		return __underlying_strncat(p, q, count);
 	p_len = strlen(p);
 	copy_len = strnlen(q, count);
-	if (p_size < p_len + copy_len + 1)
-		fortify_panic(__func__);
+	size = p_len + copy_len + 1;
+	if (p_size < size)
+		fortify_panic(__func__, 1, p_size, size);
 	__underlying_memcpy(p + p_len, q, copy_len);
 	p[p_len + copy_len] = '\0';
 	return p;
@@ -444,7 +448,7 @@  __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
 	 * lengths are unknown.)
 	 */
 	if (p_size != SIZE_MAX && p_size < size)
-		fortify_panic("memset");
+		fortify_panic("memset", 1, p_size, size);
 }
 
 #define __fortify_memset_chk(p, c, size, p_size, p_size_field) ({	\
@@ -542,9 +546,10 @@  __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
 	 * (The SIZE_MAX test is to optimize away checks where the buffer
 	 * lengths are unknown.)
 	 */
-	if ((p_size != SIZE_MAX && p_size < size) ||
-	    (q_size != SIZE_MAX && q_size < size))
-		fortify_panic(func);
+	if (p_size != SIZE_MAX && p_size < size)
+		fortify_panic(func, 1, p_size, size);
+	if (q_size != SIZE_MAX && q_size < size)
+		fortify_panic(func, 0, q_size, size);
 
 	/*
 	 * Warn when writing beyond destination field size.
@@ -644,7 +649,7 @@  __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(__func__, 1, p_size, size);
 	return __real_memscan(p, c, size);
 }
 
@@ -660,8 +665,10 @@  int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t
 		if (__compiletime_lessthan(q_size, size))
 			__read_overflow2();
 	}
-	if (p_size < size || q_size < size)
-		fortify_panic(__func__);
+	if (p_size < size)
+		fortify_panic(__func__, 1, p_size, size);
+	if (q_size < size)
+		fortify_panic(__func__, 0, q_size, size);
 	return __underlying_memcmp(p, q, size);
 }
 
@@ -673,7 +680,7 @@  void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(__func__, 1, p_size, size);
 	return __underlying_memchr(p, c, size);
 }
 
@@ -685,7 +692,7 @@  __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(__func__, 1, p_size, size);
 	return __real_memchr_inv(p, c, size);
 }
 
@@ -698,7 +705,7 @@  __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(__func__, 1, p_size, size);
 	return __real_kmemdup(p, size, gfp);
 }
 
@@ -735,7 +742,7 @@  char *strcpy(char * const POS p, const char * const POS q)
 		__write_overflow();
 	/* Run-time check for dynamic size overflow. */
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(__func__, 1, p_size, size);
 	__underlying_memcpy(p, q, size);
 	return p;
 }
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 230020a2e076..b2d3e1d3931e 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -1021,9 +1021,10 @@  EXPORT_SYMBOL(__read_overflow2_field);
 void __write_overflow_field(size_t avail, size_t wanted) { }
 EXPORT_SYMBOL(__write_overflow_field);
 
-void fortify_panic(const char *name)
+void fortify_panic(const char *name, bool dest, size_t avail, size_t len)
 {
-	pr_emerg("detected buffer overflow in %s\n", name);
+	pr_emerg("%s: detected buffer overflow: %zu byte %s buffer of size %zu\n",
+		 name, len, dest ? "write to" : "read from", avail);
 	BUG();
 }
 EXPORT_SYMBOL(fortify_panic);